Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API consistency. Use item instead of text everywhere #16821

Open
vlazar opened this issue Jan 21, 2016 · 67 comments
Open

API consistency. Use item instead of text everywhere #16821

vlazar opened this issue Jan 21, 2016 · 67 comments
Labels
Milestone

Comments

@vlazar
Copy link
Collaborator

vlazar commented Jan 21, 2016

Continues #16818

With list of string items everything works out of the box right now.

There is also a frequently requested feature to have items as objects (key/value pairs). Making it work on top of current API is problematic.

Currently we pass list item (either a string or an object) to:

// text is actually a list item, even if it's an object
filter: function (text, input)
item: function (text, input)
// a and b both items (text if list is array of strings, object if it's array of objects)
sort: function (a, b)

And here we have text (so items as object won't work):

// text here always text as it's called as this.replace(selected.textContent);
replace: function (text)
// and we also have text property in here
$.fire(this.input, "awesomplete-select", {
    text: selected.textContent,

I think this needs to be changed so that when we have list of objects both replace and awesomplete-select would also get the whole item data instead of text only. This change would make features like key/value support much easier to implement.

If we go with this, then the question is how we implement this API change.

We can make it as non breaking change in 1.x (possibly showing deprecation warning in console too):

replace: function (text, item)
$.fire(this.input, "awesomplete-select", {
    text: selected.textContent,
    item: this._list[this.index]

And cleanup API in 2.0:

replace: function (item)
$.fire(this.input, "awesomplete-select", {
    item: this._list[this.index]

However in this case replace signature changes twice. So a better option would be to introduce new item instance property, which would be a selected item data (text or object) - essentially this._list[this.index]. We can even have replace: function () without arguments in 2.0 since this.item would be accessible on any instance. So 2.0 API can be:

// just use this.item the same way this.input is used currently
replace: function ()
// still need to pass it to event handler
$.fire(this.input, "awesomplete-select", {
    item: this.item

See also related discussion #16819

@TxHawks
Copy link
Contributor

TxHawks commented Jan 21, 2016

+1

On Thu, Jan 21, 2016, 15:17 Vladislav Zarakovsky [email protected]
wrote:

Continues #16818 #16818

With list of string items everything works out of the box right now.

There is also a frequently requested feature to have items as objects
(key/value pairs). Making it work on top of current API is problematic.

Currently we pass list item (either a string or an object) to:

// text is actually a list item, even if it's an objectfilter: function (text, input)item: function (text, input)// a and b both items (text if list is array of strings, object if it's array of objects)sort: function (a, b)

And here we have text (so items as object won't work):

// text here always text as it's called as this.replace(selected.textContent);replace: function (text)// and we also have text property in here$.fire(this.input, "awesomplete-select", {
text: selected.textContent,

I think this needs to be changed so that when we have list of objects both
replace and awesomplete-select would also get the whole item data instead
of text only. This change would make features like key/value support much
easier to implement.

If we go with this, then the question is how we implement this API change.

We can make it as non breaking change in 1.x (possibly showing
deprecation warning in console too):

replace: function (text, item)$.fire(this.input, "awesomplete-select", {
text: selected.textContent,
item: this._list[this.index]

And cleanup API in 2.0:

replace: function (item)$.fire(this.input, "awesomplete-select", {
item: this._list[this.index]

However in this case replace signature changes twice. So a better option
would be to introduce new item instance property, which would be a
selected item data (text or object) - essentially this._list[this.index].
We can even have replace: function () without arguments in 2.0 since
this.item would be accessible on any instance. So 2.0 API can be:

// just use this.item the same way this.input is used currentlyreplace: function ()// still need to pass it to event handler$.fire(this.input, "awesomplete-select", {
item: this.item

See also related discussion #16819
#16819


Reply to this email directly or view it on GitHub
#16821.

@LeaVerou
Copy link
Owner

Idea: How about we pass an object that has properties with all the stuff we need, and a toString method that returns the string the 1.x API returned? That way it will be a non-breaking change for most use cases as it will return what's expected any time it's converted to a string for any purpose.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 22, 2016

Do you mean wrap item so that it'll have toString method and pass it into replace?

replace: function (item) {
    this.input.value = item.toString();
}

Need to think more on this. It might work for replace.

But we still have old text and new item property names for awesomplete-select event. So API change is about property name here. Preferably 2.0 only have an item, but path to 2.0 may require having both text and item.

$.fire(this.input, "awesomplete-select", {
    text: selected.textContent,
    item: this._list[this.index]

@LeaVerou
Copy link
Owner

You don't need to explicitly call toString. Just concatenating with an empty string works, and is less exception-prone.

@LeaVerou
Copy link
Owner

Oh, actually, nevermind. I just read this more carefully. Yes, we should have both item and text, for backwards compat. text could even be a getter that returns this.item.textContent, to avoid them getting out of sync.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 23, 2016

Great!

So we have origin property already merged in 5abf4df#diff-0861bfe5a8e20a86e84f647fe9e26358R205

We have text already for selected item text.

Looks like you agree to use item as property name for item object. BTW item will make #16795 cleaner, than with selected DOM element passed in.

@LeaVerou Which leaves only 1 more requested property without chosen name - the selected LI element.

I propose to use element for property as the element in awesomplete-select or awesomplete-selectcomplete. I think element will have clear meaning of selected element. We can also use selected if you think it's better. Or any better name if you have one in mind.

// already names properties
text: itemText,
item: itemTextOrItemObject,
origin: clickedChildOfItemElement

// NEED TO NAME THIS ONE!
element: selectedItemElement, // or maybe `selected` (but to me `origin` aligns better with `element`)

@LeaVerou
Copy link
Owner

Why can't this be the event target?

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 24, 2016

Event target for awesomplete-select and awesomplete-selectcomplete is input element, not LI element.

@LeaVerou
Copy link
Owner

Oh right. It's been a while!
Ok then, origin sounds fine :)

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 24, 2016

The origin name is already in use and merged. The question is how to name selected LI element property for awesomplete-select and awesomplete-selectcomplete events.

I've come up with element. Maybe you have better name for it. Another idea was selected, but if we also add item as agreed before, I think element is better name.

I'm experimenting with selectedItem/selectedElement getters now, to see if this make code better. Now we get item data via selected.textContent in some places (query DOM), but we can get it directly from list. This will also work when items list is objects instead of strings.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 24, 2016

Something like this:
vlazar@a18f32b

  • uses original list data instead of querying DOM with el.textContent for selected item text
  • replace() gets original item data (can be object with key/value pairs), not only selected text
  • text property in awesomplete-select also get item data; can change this as discussed of course to have both text and item properties for backwards compatibility

This is just to get an idea of how it can be implemented.

@LeaVerou
Copy link
Owner

Wait, isn't item and element the same thing? What's their difference? If I'm that confused, I'm worried our users will be way more confused...

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 28, 2016

Well, this is what all issue about. We need good names.

As we discussed already both item data (text for strings and item for objects) and DOM elements (origin for exact clicked element, somenameweneedtochose for selected LI) are useful. Personally I think the most valuable addition would be item for data, but I can see elements can be useful too, just not as much as item with data.

Or do you referring to my idea in this commit? vlazar@a18f32b#diff-0861bfe5a8e20a86e84f647fe9e26358R143

I don't like selectedItem and selectedElement getter names. I'm just trying getters to see if it is a good idea. It looks like we agreed we need item data in events, so it might be useful to have them both internally and as public API.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 29, 2016

We can use li instead if element for awesomplete-select and awesomplete-selectcomplete events and this.li or this.selectedLi instance property instead for this.selectedElement currently shown in vlazar@a18f32b#diff-0861bfe5a8e20a86e84f647fe9e26358R147

With li as base name for selected DOM element

Event properties:

// existing property, selected text (does not work for objects)
text: itemText,

// existing property, exact clicked DOM element
// can be a child of LI if clicked with mouse or LI when selected with keyboard
origin: clickedChildOfItemElement

// new property
// string, if list was initialized with array of strings
// object, if list was initialized with array of objects instead of strings
// example: { id: 1, title: "JavaScript" }
item: itemTextOrItemObject,

// new property
// selected LI element
// Personally I don't think it's very useful, especially with existing origin property
// and if we add item property mentioned above. It's just that others requested it
// and there is no element.closest() in IE to get it with origin.closest("LI")
li: selectedItemElement

Instance properties:

// note: item name already taken for render item function
// use select prefix to align with other similar properties
get selectedItem() {
    return this._evaluatedList[this.index];
},
get selectedLi() {
    return this.ul.children[this.index];
},

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 29, 2016

One more combination on names.

Event properties:

text: itemText,
value: itemTextOrItemObject,
origin: clickedChildOfItemElement
element: selectedItemElement

Instance properties:

get value() {
    return this._evaluatedList[this.index];
},
get element() {
    return this.ul.children[this.index];
},

vlazar referenced this issue in vlazar/awesomplete Jan 29, 2016
@vlazar
Copy link
Collaborator Author

vlazar commented Jan 29, 2016

OK. Smaller changes are easier to follow. Here is what it roughly takes to pass item data instead of just text everywhere: vlazar@b7adf40

Later we can also add getter.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 29, 2016

One more idea.

If data required for events are also available as instance property we can add just one property - instance.

$.bind(input, { "awesomplete-select": function (evt) {
  // Awesomplete instance
  evt.instance;

  // selected LI element
  evt.instance.ul.children[evt.instance.index];
  // or with getter
  evt.instance.element;

  // selected data object
  evt.instance.suggestions[evt.instance.index];
  // or with getter
  evt.instance.value;
});

@LeaVerou
Copy link
Owner

Ok, I see now.

How about:

text -> text
origin -> originalTarget
item -> data
li -> element

?

Definitely not li. It would be extremely misleading if people used any other element.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 29, 2016

Not sure about originalTarget since it's non standard and doesn't seem to become as stated here https://developer.mozilla.org/en-US/docs/Web/API/Event/originalTarget

Was only selected li since we already have this.ul.

@LeaVerou
Copy link
Owner

Oh, I didn't even remember there was a de facto standard about it, I was just suggesting it as our own property :)
this.ul was a bad choice too, but too late for that I guess...

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 30, 2016

I can change origin to originalTarget. Maybe I misunderstood, but it looks like we agreed on origin name here #16819 (comment) It's already merged in https://github.com/LeaVerou/awesomplete/pull/16823/files#diff-0861bfe5a8e20a86e84f647fe9e26358R205

I'll start adding data for selected item data and element for selected item element. Do you think it will be useful to also add instance properties?

get data() {
    return this._evaluatedList[this.index];
},
get element() {
    return this.ul.children[this.index];
},

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 30, 2016

Here is the PR for item data property #16835

Please consider addition of related instance property as well. And if the name data is final let's merge it and move to element property with same question - should we add related instance property as well?

@vlazar vlazar added this to the v1.1 milestone Jan 31, 2016
@LeaVerou
Copy link
Owner

LeaVerou commented Feb 5, 2016

Yes, definitely. I propose awesomplete as a name for that property.

@ghost
Copy link

ghost commented Feb 5, 2016

I assume I have to know logins to unsubscribe even if that was an option. Any value prop of github and your project was immediately lost with the automatic transmission of every message since the dawn of man

Thank you,
Stacey

On Feb 4, 2016, at 7:29 PM, Lea Verou [email protected] wrote:

Yes, definitely. I propose awesomplete as a name for that property.


Reply to this email directly or view it on GitHub.

@LeaVerou
Copy link
Owner

LeaVerou commented Feb 5, 2016

Github is just the site hosting my project, I do not control what they do with email notifications, nor did I make the site. I’m just trying to be helpful here. You (well, more likely someone with access to your email) subscribed to this project, which is why you're getting the notifications. The username I see is Sundevil96 and their photo is this one if it rings any bells. Perhaps your son or partner?

You can try clicking "forgot your password?" on the login page, so that you can reset the password via email. And seriously, don't let other people use your email to avoid such problems in the future. :)

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 17, 2016

So how we fix it? Just allow to override any read only property on Event?

$.fire = function(target, type, properties) {
        evt.initEvent(type, true, true);

        for (var j in properties) {
-               evt[j] = properties[j];
+               Object.defineProperty(evt, j, {
+                       value: properties[j],
+                       configurable: true, writable: true, enumerable: true
+               });
        }
    return target.dispatchEvent(evt);
};

I haven't tested all read only Event properties, but overriding at least cancelable and bubbles does nothing to Event behavior. Initially set to true with evt.initEvent(type, true, true) those properties can be changed to false with Object.defineProperty, but event will still bubble and we can still do preventDefault().

We can at least special case cancelable and bubbles so that value and behavior won't mismatch:

+               if (j == "cancelable" || j == "bubbles") continue;
                Object.defineProperty(evt, j, {

@LeaVerou
Copy link
Owner

You can try setting and defineProperty if it fails. Otherwise even if an accessor has a getter, you're not using it (case in point with cancellable and bubbles).

@LeaVerou
Copy link
Owner

Or we can just use a different name, I give up. This is too much work just to use originalTarget. Let's just call it origin or whatever you want and call it a day.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 17, 2016

Or maybe it's ok to just use Object.defineProperty without any checks? Does anything bad can happen with some read only event properties changed?

@LeaVerou
Copy link
Owner

No, it is not. evt.bubbles for example is a setter. When you set that property, you don't just set a property, there is code that runs behind the scenes. If you overwrite it with a simple property, that code will not run any more.
Let's just call it origin or whatever and move on. It's really not worth the trouble or bytes just to have a name that vaguely reminds of other event properties.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 17, 2016

Reverted in dfd16c9. Didn't knew there are some writable event properties.

@LeaVerou
Copy link
Owner

Thanks, and sorry for the back and forth on this. I still think that originalTarget would have been a better name, but it's simply not worth all this trouble.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 17, 2016

And we need to decide on others as well. Did I understood you correctly in my comment above? #16821 (comment)

Now we have text, data and origin event properties. The data was added recently as part of work on key/value story (it has the same value as text though).

So we keep just the text, origin and provide access to awesomplete instance from event, so that any instance data can be accessed in event handler? Does it mean we remove data?

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 18, 2016

Also, if we going to provide access to Awesomplete instance from the event... maybe instead of adding awesomplete property to each event we can instead add awesomplete property to <input>? This will also help with check if Awesomplete was already created for this input. See related but #16836 (comment)

this.input.awesomplete = this;

If it's the only property we will add to input... then it should be fairly safe to do as it won't leak nowadays? Or do we still need an auto incremented string property on DOM element and using cache object in JS like jQuery and others do since like forever?

@LeaVerou
Copy link
Owner

My main worry is that some people tend to get a bit uncomfortable when you add properties to DOM elements, due to the old IE memory leaks (which are completely obsolete by now but the aversion remains because humans are not rational). But apart from that, that's great. I wonder if anyone else has any opinions?

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 18, 2016

Lea, can you please also answer my previous question, while you are here? :) #16821 (comment)

@LeaVerou
Copy link
Owner

and using cache object in JS like jQuery and others do since like forever?

or we can use ES2015 maps 😈

@LeaVerou
Copy link
Owner

So we keep just the text, origin and provide access to awesomplete instance from event, so that any instance data can be accessed in event handler? Does it mean we remove data?

I'm leaning towards it, but no strong opinions there. Up to you. :)

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 18, 2016

I'm leaning towards it, but no strong opinions there. Up to you. :)

If I understood you correctly, you are not in favor of removing text property even in 2.0 since we don't want to break things. So if we are not switching to data from text in 2.0, there is no reason to have both.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 18, 2016

or we can use ES2015 maps 😈

I'm all for IE11+ :) Would make other parts much easier too.

@LeaVerou
Copy link
Owner

If I understood you correctly, you are not in favor of removing text property even in 2.0 since we don't want to break things. So if we are not switching to data from text in 2.0, there is no reason to have both.

Ok!

I'm all for IE11+ :) Would make other parts much easier too.

Hmm. Let me think about it for a bit.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 19, 2016

Turns out Map/WeakMap support is not so great in other browsers too. Probably not an option in the near future.

As for old IEs, we can just wait until IE10 usage drops below 2% :)

@LeaVerou
Copy link
Owner

IE10 usage is 0.81%. Same with IE9. IE8 is 1.02%. I've stopped worrying about them in my own projects, but still try to support them in libraries mainly because other people care (probably due to misinformation propagating: Either they or their bosses/clients think IE<10 is way more popular than it is. And there are of course also the huge sites that actually should care).

@TxHawks
Copy link
Contributor

TxHawks commented Feb 19, 2016

WeakMap can be polyfilled (and one is available through polyfill.io), but
it seems like map cannot be.

On Fri, Feb 19, 2016, 08:59 Lea Verou [email protected] wrote:

IE10 usage is 0.81%. Same with IE9. IE8 is 1.02%


Reply to this email directly or view it on GitHub
#16821 (comment)
.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 19, 2016

IE10 usage is 0.81%. Same with IE9. IE8 is 1.02%.

What source do you use?

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 19, 2016

WeakMap can be polyfilled (and one is available through polyfill.io), but
it seems like map cannot be.

Interesting. I would rather expect the opposite.It might have to do with a desire to limit the size of polyfills. One could just use WeakMap, no big need to polyfill Map too.

@LeaVerou
Copy link
Owner

What source do you use?

caniuse, which gets its data from StatCounter.

Interesting. I would rather expect the opposite.

Same here!

Btw note that I'm not saying we should use Map. People do care about IE <= 10 (even though in most cases they shouldn't) and I'd rather not depend on polyfills. We could keep it in mind for the future though.

@vlazar
Copy link
Collaborator Author

vlazar commented Feb 19, 2016

With official support dropped in January I hope IE10 memories will die pretty soon 😈

@LeaVerou
Copy link
Owner

Fingers crossed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants