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

Component event strategy #26

Open
geoffrich opened this issue Apr 26, 2021 · 8 comments
Open

Component event strategy #26

geoffrich opened this issue Apr 26, 2021 · 8 comments

Comments

@geoffrich
Copy link
Contributor

Is your feature request related to a problem? Please describe.

It's tricky listening to component events in React. Sometimes inline handlers work (e.g. onInput), and sometimes you have to have to attach event listeners using a ref. Because of React's synthetic event system, you can only use inline handlers if the event is a native DOM event. If it's a custom event, you have to use a ref.

auro-radio is tricky. The input event emitted from the component is from the inner input. This is because input is a composed event -- it propagates across shadow DOM boundaries. Because it's native, you can listen to it using onInput. change is different. The native change event is not composed, so we re-dispatch it as a custom event. Because of this, you can't listen to the change event using onChange in React and you have to use a ref.

This is confusing, and it's not clear to the consumer that there's a difference.

Describe the solution you'd like

We should distinguish custom component events from native ones so that it's clear there's a difference.

Instead of dispatching a change event, we dispatch an auro-change event instead. This makes it clear that this is a custom event instead of a native event. It's also similar to how other component libraries name their events.

I'm still thinking through whether we'd want to name all component events like this. The escaping input event is really a leaky abstraction -- should the consumer care how the component is implemented under the hood? Or do we tell them what events to listen to? For instance, Shoelace makes heavy usage of custom events.

Either way, by prefixing our custom events, we make it clear to consumers that they are custom events. If the event begins with auro-, they should assume that they need to use a React ref to set up event listeners instead of an inline listener.

Some related thoughts from Westbrook: composed: true considered harmful?.

Describe alternatives you've considered

Instead of changing component behavior, we simply document the difference between the events and what it means for React.

@geoffrich geoffrich added the Type: Feature New Feature label Apr 26, 2021
@blackfalcon blackfalcon added the help wanted Extra attention is needed, this user requires assistance to complete the work label Apr 26, 2021
@blackfalcon
Copy link
Member

Meeting to review this feature request has been scheduled.

@blackfalcon
Copy link
Member

100% agree with the idea that we are creating custom elements and by extension creating custom events are expected but embraced in this community.

A couple of things.

  1. This element, as well as auro-checkbox, are up for massive functional and feature updates. It would be best to kick this can down the road and coordinate a MAJOR release that addresses this concern.
  2. See Generator: custom events update to Auro guide to API development WC-Generator#155

@blackfalcon
Copy link
Member

Either way, by prefixing our custom events, we make it clear to consumers that they are custom events. If the event begins with auro-, they should assume that they need to use a React ref to set up event listeners instead of an inline listener.

I really like the clarity of this versus trying to get events to appear as native HTML, which we are not using.

In regard to the React Ref, I am hoping we can obscure that with the future migration to Lit2.0 and React wrappers.

@blackfalcon
Copy link
Member

@geoffrich so that we are on the same page, what is it in THIS repo that you specifically want to see updated? Assuming that we are talking about the following lines? https://github.com/AlaskaAirlines/auro-radio/blob/master/src/auro-radio.js#L60-L82

While we agree conceptually, this issue does not have an expectation and/or exit criteria.

@blackfalcon blackfalcon removed the help wanted Extra attention is needed, this user requires assistance to complete the work label Jun 11, 2021
@geoffrich
Copy link
Contributor Author

The existing custom events are renamed and become the preferred way to interact with the component.

old event new event
change auro-change
toggleSelected auro-input
focusSelected auro-focus

This will be a breaking change.

@blackfalcon blackfalcon removed their assignment Jun 11, 2021
@blackfalcon blackfalcon self-assigned this Jun 23, 2021
@blackfalcon blackfalcon changed the title auro-radio: component event strategy [linked]: component event strategy Aug 18, 2021
@blackfalcon blackfalcon changed the title [linked]: component event strategy Component event strategy Aug 18, 2021
@blackfalcon blackfalcon removed their assignment Aug 24, 2021
@blackfalcon blackfalcon added this to the v2.0-rc milestone Sep 6, 2021
@braven112 braven112 removed the Issue label Dec 21, 2021
@blackfalcon
Copy link
Member

This issue is bound to AlaskaAirlines/WC-Generator#224

@blackfalcon
Copy link
Member

blackfalcon commented Feb 3, 2022

Had a conversation with @geoffrich and while there is an opportunity to update this API, it's not a critical issue to address. Teams who have consumed this element in React have engineered wrappers that address this shortcoming. There is also the opportunity that updates in React will nullify the need for this as well ¯_(ツ)_/¯

The concept of hijacking native events has been a decision that was made when this all started... that being said, had we not done that, there are things that would have been a lot easier to do as well (auro-input and the webkit cursor issue).

While we agree that there is something that can be done here, I'd like to take into consideration all the other issues around events and reactive code and propose that there is a concentrated effort to update all the form elements with new form support in the browser pertaining to web components.

@blackfalcon
Copy link
Member

I want to get this back into the rotation so that we can discuss this update and how it related to the larger goal of more reactive form elements with the new formData API opportunities.

@blackfalcon blackfalcon removed this from the auro-radio v2.0-rc milestone Jan 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants