Spread list props to suggestion

[Secretmapper]

Spread all props of list object to Suggestions object.

Super direct way and not sure if it breaks backwards compat/works in all browsers

Adresses #16976, #16938, #16888

[vlazar]

Thanks for your contribution!

I think Array and non label/value Object cases can be skipped. No need to copy properties when data is Array or Object without label/value properties.

Also we shouldn't probably copy all properties from objects. We inherited Suggestion object from String and re-defined some properties. Probably not all String features currently work for Suggestion, but overriding all properties on Suggestion blindly can probably lead to more potential issues.

@LeaVerou cares a lot about backward API compatibility. We should not break Awesomplete API.

[Secretmapper]

@vlazar Good point. I didn't even realize Suggestion inherits from String.

Perhaps a better solution then is to namespace the passed object as a prop, ie this.data = data and the object itself can be accessed from the suggestion. What do you guys think?

[vlazar]

I was thinking about having suggestion.data for all other props as a way to not complicate implementation, but then API becomes weird:

suggestion.label
suggestion.value
suggestion.data.everythingElse

Of course you can just use:

suggestion.data.label
suggestion.data.value
suggestion.data.everythingElse

But once introduced, this API would probably have to stay forever.

Lea, what do you think?

[TxHawks]

+1 for

suggestion.data.label
suggestion.data.value
suggestion.data.whatever

On Thu, Nov 10, 2016, 09:31 Vladislav Zarakovsky [email protected]
wrote:

I was thinking about having suggestion.data for all other props as a way
to not complicate implementation, but then API becomes weird:

suggestion.labelsuggestion.valuesuggestion.data.everythingElse

Of course you can just use:

suggestion.data.labelsuggestion.data.valuesuggestion.data.everythingElse

But once introduced, this API would probably have to stay forever.

Lea, what do you think?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#17001 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AFZXkvdKMO8D2DdvnlQ7MxapAn0c-19Zks5q8shFgaJpZM4KraAT
.

[joonassandell]

Any updates on this?

[vlazar]

@LeaVerou What do you think?

[LeaVerou]

@LeaVerou cares a lot about backward API compatibility. We should not break Awesomplete API.

To be clear, it's not backwards compatibility I care so much about, it's usability. Awesomplete's main use case is people using it with a list of strings. Allowing more complex use cases should NEVER complicate the simple, common case. That's how people end up with terrible APIs that are a pain to use: They design the API based on the most complicated cases they want to support, and its gets in the way of the majority of their users with simpler use cases. Good APIs make the complex possible and the simple easy, not just one of the two.

That said, I can't see how this PR could break anything. Unless the properties of the object passed in happen to have the same name as string methods that are used, there shouldn't be any issue.

I would have some stylistic comments though:

1. Please follow the style of the rest of the code, with blocks being on separate lines even if they only have one line.
2. Please rename attr to prop or (better) property. These are not attributes, they are properties. Attributes can be confused with HTML attributes.
3. As @vlazar mentioned, you don't need this unless data is an object. Since there is another check about whether this is the case, we should convert it to a proper block if and do it in there.

[vlazar]

@LeaVerou How about this idea? #17001 (comment)

[LeaVerou]

@LeaVerou How about this idea? #17001 (comment)

The indirection seems like a hassle IMO. The chances of both having a conflict and people trying to use the String method/property that is obscured by the data are pretty slim if you ask me, and easy to fix by just renaming the property in their data.

[Lesha-spr]

Any updates on this?

[bholtbholt]

@LeaVerou if this is something still worth adding I'm happy to take it over the finish line. It fits a use-case that's coming up for me!

I think I'll need to open a new PR, but I've put comment suggestions here as well.

[bholtbholt]

• Please follow the style of the rest of the code, with blocks being on separate lines even if they only have one line.
• Please rename attr to prop or (better) property. These are not attributes, they are properties. Attributes can be confused with HTML attributes.

Suggested change

	for (var attr in data) { this[attr] = data[attr]; }
	if (typeof data === "object") {
	for (var property in data) {
	this[property] = data[property];
	}
	}

[bholtbholt]

• As @vlazar mentioned, you don't need this unless data is an object. Since there is another check about whether this is the case, we should convert it to a proper block if and do it in there.

Suggested change

	: typeof data === "object" && "label" in data && "value" in data ? data : { label: data, value: data };
	if (Array.isArray(data)) {
	o = { label: data[0], value: data[1] }
	} else if (typeof data === "object") {
	for (var property in data) {
	this[property] = data[property];
	}
	} else if (typeof data === "object" && !("label" in data && "value" in data)) {
	o = { label: data, value: data }
	}