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

React 17 Compatibility #4028

Closed
brianespinosa opened this issue Aug 10, 2020 · 8 comments · Fixed by #4031
Closed

React 17 Compatibility #4028

brianespinosa opened this issue Aug 10, 2020 · 8 comments · Fixed by #4031

Comments

@brianespinosa
Copy link
Member

I think it would be good for us to potentially track any issues here surrounding React 17. The good news is this release is intended to move things forward progressively.

It seems like the only thing that might be worrying to us is no more event pooling optimization. I know that we did at some point add in some functionality to help with this using eventStack but I am not sure if this change in 17 will impact us due to this.

@layershifter since you are the author of that, you are probably best to answer if this has any impact for us in SUIR. If it is irrelevant, I think we could probably close this whole issue based on the rest of the release notes for this RC.

@layershifter
Copy link
Member

It should not be an issue for EventStack as it works with DOM events, not with React's.

However, with have at least one usage of the described behavior:

closeWithTimeout = (e, delay) => {
debug('closeWithTimeout()', delay)
// React wipes the entire event object and suggests using e.persist() if
// you need the event for async access. However, even with e.persist
// certain required props (e.g. currentTarget) are null so we're forced to clone.
const eventClone = { ...e }
return setTimeout(() => this.close(eventClone), delay || 0)
}

@layershifter
Copy link
Member

To be clear: EventStack should be removed with a cleaner alternative, but it's a separate discussion.

@layershifter
Copy link
Member

I created #4031 to run CI with React 17, but it seems that it will not be easy as Enzyme has issues with React 17:

image

@layershifter
Copy link
Member

enzymejs/enzyme#2429 😨

@brianespinosa
Copy link
Member Author

enzymejs/enzyme#2429 😨

Yikes

@brianespinosa brianespinosa linked a pull request Aug 11, 2020 that will close this issue
@layershifter
Copy link
Member

layershifter commented Aug 26, 2020

Small update from #4031.


Deployment to Vercel shows running doc site and the test suite is passing except few tests, but it related to Enzyme as cleanup functions in hooks are executed after wrapper.unmount(). It seems that everything works 👾

@layershifter
Copy link
Member

React 17 was released. I will fix peerDependencies to avoid warnings, this will be included to a next release.


Our tooling for doc site is super outdated (mainly because we are using react-static as SSG), that brings a set of complications, however it's not related to published package on NPM. I want migrate to Next.js 🙏

@brianespinosa
Copy link
Member Author

@layershifter agree on migrating to Next.js for the docs. I have actually put an entire doc site together with Next.js using react-docgen and next-mdx-remote. It's pretty impressive stuff. Let me know if you want to chat on getting that set up. I am guessing it makes sense to do that work separately and get it running, then we can flip the switch over to the new docs.

I saw @levithomason did something with a branch that might have looked like it was using GH pages a month or two ago... so I am not sure what we want to do exactly. I could also be wrong on seeing that branch happen or what it might have been for.

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

Successfully merging a pull request may close this issue.

2 participants