✨ amp-autocomplete: Add support for making object items available in autocomplete select events

[joaopsrleal]

Introduces a new data-json attribute to support sending objects as the event.value of select & change events. This requires adding a objToJson attribute to the objects passed to the mustache templates used to render the autocomplete suggestions.

Resolves #26525.

[CLAassistant]

All committers have signed the CLA.

[micajuine-ho]

Hi @joaopsrleal, thanks for your contribution!

Just to be clear, this change is only applicable for client-side filtering right?

[joaopsrleal]

Hi @joaopsrleal, thanks for your contribution!

Just to be clear, this change is only applicable for client-side filtering right?

Thank you! :)

That's a good point. I hadn't considered the server-side filtering case. I think we should support it as well. To do that, we'd only have to do the same massaging of the data here. Any concerns?

Also, I was thinking: right now users have to call data-json and pass the correct name in {{...}} as the value. Something we could consider is to somehow move some of this logic to the template service and auto-expand the template. For instance, this way users would only have to specify data-json (or we can choose another name for the attribute) and then we'd auto expand it to data-json={{objToJson}} in the element's DOM. It would definitely make it easier to use this feature. Let me know whether you think this would be worth it and I'm happy to explore it a bit more.

[caroqliu]

Hi @joaopsrleal, thanks for making this contribution! I wanted to ask if it is necessary to add an attribute to expose the new value. It sounds like the proposed approach is to use the attribute to expose the full event object on event.value instead of the data-value of the selected element. What do you think about keeping event.value as is, and exposing another event property additionally in all cases? I'm thinking something like event.valueAsObject, similar to how input elements expose event.valueAsNumber.

@micajuine-ho What do you think?

[micajuine-ho]

@micajuine-ho What do you think?

I like this approach better as it better aligns with the existing API.

That's a good point. I hadn't considered the server-side filtering case. I think we should support it as well. To do that, we'd only have to do the same massaging of the data here. Any concerns?

I just wanted to make sure that there was no overlap with the changed code and the SSR case.

As for adding this as a feature here, I think I would prefer if we continue with the client-side filtering case only, and then if SSR case is requested, then we can add in that feature. I think it's also safer for bugfixes/rollbacks.

[joaopsrleal]

Hi @joaopsrleal, thanks for making this contribution! I wanted to ask if it is necessary to add an attribute to expose the new value. It sounds like the proposed approach is to use the attribute to expose the full event object on event.value instead of the data-value of the selected element. What do you think about keeping event.value as is, and exposing another event property additionally in all cases? I'm thinking something like event.valueAsObject, similar to how input elements expose event.valueAsNumber.

@micajuine-ho What do you think?

That's an interesting idea.

Regarding whether it is needed to add an attribute to expose the new value: we need a way of correlating the element on the DOM (which in this case is the rendered autocomplete suggestion) with the original data item, since we currently use what is on the DOM for determining the event.value. The safest way of doing that is by embedding this information on the elements' DOM. Assuming the elements returned here respect the order of filteredData (may be a brittle assumption), we could modify the element by adding either the json element like I'm doing now or a number representing the index of the data element it represents. This wouldn't require users to add the element themselves. Maybe there's another way of doing it, but I couldn't come up with any other ideas :/. I'd love to hear your thoughts on this.

In any case, regardless of whether we use an attribute, I believe we could extend this method to pass a valueAsObject attribute to the created event in the cases it makes sense. I'll make that change.

[joaopsrleal]

Hi @joaopsrleal, thanks for making this contribution! I wanted to ask if it is necessary to add an attribute to expose the new value. It sounds like the proposed approach is to use the attribute to expose the full event object on event.value instead of the data-value of the selected element. What do you think about keeping event.value as is, and exposing another event property additionally in all cases? I'm thinking something like event.valueAsObject, similar to how input elements expose event.valueAsNumber.
@micajuine-ho What do you think?

That's an interesting idea.

Regarding whether it is needed to add an attribute to expose the new value: we need a way of correlating the element on the DOM (which in this case is the rendered autocomplete suggestion) with the original data item, since we currently use what is on the DOM for determining the event.value. The safest way of doing that is by embedding this information on the elements' DOM. Assuming the elements returned here respect the order of filteredData (may be a brittle assumption), we could modify the element by adding either the json element like I'm doing now or a number representing the index of the data element it represents. This wouldn't require users to add the element themselves. Maybe there's another way of doing it, but I couldn't come up with any other ideas :/. I'd love to hear your thoughts on this.

In any case, regardless of whether we use an attribute, I believe we could extend this method to pass a valueAsObject attribute to the created event in the cases it makes sense. I'll make that change.

I've just made the change to make .valueAsObject available in the event next to .value. They should both be available now.

Let me know whether you have any thoughts regarding the need for additing the data attribute. If we think it's safe to assume that the order of the rendered elements is the same as the provided data elements, we could hide that from users. However, I feel this part of the code may become too brittle. Other ideas are welcome.

[joaopsrleal]

@micajuine-ho What do you think?

I like this approach better as it better aligns with the existing API.

That's a good point. I hadn't considered the server-side filtering case. I think we should support it as well. To do that, we'd only have to do the same massaging of the data here. Any concerns?

I just wanted to make sure that there was no overlap with the changed code and the SSR case.

As for adding this as a feature here, I think I would prefer if we continue with the client-side filtering case only, and then if SSR case is requested, then we can add in that feature. I think it's also safer for bugfixes/rollbacks.

The original issue #26525 doesn't specifically request this for client-side rendering only. I imagine that many autocomplete use cases rely on SSR, which means this feature won't initially be available to them. If we decide to go with just client-side rendering, I'll update the documentation to mention that this is currently only supported for client-side rendering to properly set expectations.

@caroqliu you did the original triaging of the issue, so you may have additional thoughts here.

[joaopsrleal]

Sorry for closing the PR. I mispressed a button :(
I've reopened it again.

[micajuine-ho]

Awesome, the changes look great!

[micajuine-ho]

Looks like the validator is having some issues; can you please run gulp validator --update_tests and update the PR?

[joaopsrleal]

Looks like the validator is having some issues; can you please run gulp validator --update_tests and update the PR?

Since I'm currently working on a Windows machine running gulp validator --update_tests is not possible :/
I've been fighting with it, and got it almost running but I am having problems with the line-endings.... I had to update the output file manually sadly. Now it should be working :)

[joaopsrleal]

Awesome, the changes look great!

Thanks for your review! :)

[micajuine-ho]

/cc @ampproject/wg-caching for validator test approval.

[honeybadgerdontcare]

validation changes look fine