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

fix: change to use __reactEventHandlersXXX to get latest event handler #12

Merged

Conversation

newying61
Copy link
Contributor

@newying61 newying61 commented Feb 16, 2019

Found an issue when using useState hook (React 16.8.2).
The event handler inside memoizedProps is not updated synchronously, as a result, it's using an old scope which is using the old state value.
Change to use the handlers in __reactEventHandlers from React.js ver 16.0.0, which seems to be updated every time.

Will do more test later.
Raise a pull request first, in case any one has the same issue.

Other findings:
If render react app to this.shadowRoot, we don't need to use this package.
Events are working in React 16.8.2.
But if adding a div to this.shadowRoot and render react app into it, still need this package to retarget react events.

Copy link

@AndreyLuzinov AndreyLuzinov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that fix works.

@chgohlke
Copy link
Member

@newying61 can you solve the conflicts, then i would merge your PR.

…iately, change to use __reactEventHandlers to trigger the latest event handler
@newying61 newying61 force-pushed the change_to_use_react_event_handlers branch from 80a5a5c to 993e08d Compare April 29, 2020 09:11
@newying61
Copy link
Contributor Author

@chgohlke Resolved. Thanks for the review.

@chgohlke chgohlke merged commit 516dafb into spring-media:master Apr 29, 2020
@gilles-yvetot
Copy link

@newying61 you mentioned events working with React 16.8.2. I am using 16.13.1 and try to not use this package but it was not working :( Are you sure it is working for you?

 ReactDOM.render(
        <InjectApp store={configureStore(initialState)} />,
        omniContainer.shadowRoot,
      )

@newying61
Copy link
Contributor Author

newying61 commented May 29, 2020

@gilles-yvetot It's working in my test app with latest react version 16.13.1

const rootEl = document.getElementById('root');
rootEl.attachShadow({ mode: 'open' });

ReactDOM.render(<App />, rootEl.shadowRoot);

The app looks like:

    const [state, setState] = useState(1);

    return (
      <>
        <button onClick={() => setState(state + 1)}>Increase</button>
        <div>{`Count: ${state}`}</div>
      </>
  );

Is the event not working?
If you render into omniContainer, is everything working?

In React-DOM 16.13.1, the attaching event listener code is not changed.
It's still trying to find DOCUMENT_NODE or DOCUMENT_FRAGMENT_NODE, should work with shadowRoot (document-fragment type).

@gilles-yvetot
Copy link

@newying61 sorry I am only replying now.

I tried your solution on my project but could not make it work. Then tried again on a small project and it worked. So after some debugging time, I found out that my issue was caused by a portal within the shadow dom. Outside of that, your tip helped so much. Thanks again!

@newying61
Copy link
Contributor Author

newying61 commented Aug 14, 2020

I tried your solution on my project but could not make it work. Then tried again on a small project and it worked. So after some debugging time, I found out that my issue was caused by a portal within the shadow dom. Outside of that, your tip helped so much. Thanks again!

Hi @gilles-yvetot , no problem.
oh, I had the same issue before. It seems react portal is trying to find the nearest Document element to attach the event listener. (shadowRoot is document-fragment type).
And some of the libs are rendering Portal inside document.body by default. They usually provide a rootElement prop for controlling where to render.

And some other interesting finds:

  • if the event listener is already attached to shadowRoot, for example already a button in the main app rendered and using onClick, then when rendering portal, react will not attach the event handler again. => onClick will work inside Portal as well
  • if the event handler is not already attached, like onChange for input in Portal, but not used before in the main app, react will attach the change event to nearest document => onChange will not work in Portal inside shadowRoot

So, if you know what events will be triggered in Portal inside shadowRoot, you can attach the event handler first in the main app, like
onChange={() => {}}
then it will be working fine.

@newying61
Copy link
Contributor Author

Hi @gilles-yvetot
Forgot to mention one thing, the new React ver 17.0 (current in RC) has changed the event system. It attaches one event listener to the react app render root, not document anymore.
I think this will make this package obsolete.
So using shadow DOM will be easier :)

@gilles-yvetot
Copy link

@newying61 even if your package become obsolete, thanks for the help you gave me here and to the community with this package

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

Successfully merging this pull request may close these issues.

None yet

4 participants