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 18 Support #89

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

wSedlacek
Copy link
Contributor

This adds primilar support for React 18.
It is NOT thoroughly tested and needs more extensive test cases.
It is still using the Legacy Root

@shirakaba
Copy link
Owner

shirakaba commented Jul 31, 2022

Thank you very much for this!

As mentioned on Discord, I'll be busy preparing a talk for the next two weeks so I can't immediately jump on it.

Some quick impressions:

  1. Good job on updating the @types/react patchfile.
  2. I can't read the package-lock.json diff easily because the lockfile format changes. Can we do this PR using an older npm version to simplify the diff? That, or (before this PR) we can reinstall the existing deps using a newer npm version to get the lockfile in line.
  3. There are many changes that do not relate to React 18 here. For sure, this repo needs a cleanup and a proper linting/prettier setup (up until now I've been running prettier manually, which is obviously barely a workflow), but running prettier and deleting comments makes this PR much harder to study. Can we keep the changes in this PR to just the ones relevant to React 18, please?
  4. Is changing node.getAttribute() and node.setAttribute() to direct property accesses related to React 18?
  5. I see that sample/app was moved to sample/src. Again, this seems unrelated to React 18 and is probably to do with Webpack 5. I've done that migration already on the feat/v8 branch (which I'd intend to merge before tackling React 18 support), so I should probably just merge that to master at this point to reduce the diff from this incoming PR.
  6. I notice that these lines were removed from HostConfig.ts:
    scheduleDeferredCallback: scheduler.unstable_scheduleCallback,
    cancelDeferredCallback: scheduler.unstable_cancelCallback,
    
    // @ts-ignore not in typings
    schedulePassiveEffects: scheduler.unstable_scheduleCallback,
    cancelPassiveEffects: scheduler.unstable_cancelCallback,
    
    // @ts-ignore not in typings
    shouldYield: scheduler.unstable_shouldYield,
    now: scheduler.unstable_now,
    I originally had to put them in to support hooks (React 16, perhaps?). Obviously they were all marked as unstable, so maybe things are done differently now, but do hooks still work?
  7. I saw shouldDeprioritizeSubtree was removed from HostConfig.ts as well; what's the reasoning behind that?
  8. Seeing some new APIs filled in the HostConfig gives me some confidence. What repository/guide did you reference to learn how to introduce React 18 support?
  9. Have you tested any React 18-specific features following these changes (and do they work)?
  10. I'm unfamiliar with LegacyRoot vs. ConcurrentRoot, so we'll need to do some more investigation there.

So I think, in summary, the actionables are:

  • Reduce this PR to just the React 18-specific features (i.e. don't apply prettier or remove comments as we can do that in a follow-up);
  • Use npm 6 to install the dependencies, to keep the package-lock.json consistent;
  • Rebase the PR onto feat/v8 (or wait for feat/v8 to be merged into master, then rebase onto that) as that will be landed first;
  • Add some test components that demonstrate usage of React 18 features.

I understand it's a lot of work, but thank you very much for taking the time to look into it!

shirakaba added a commit that referenced this pull request Aug 17, 2022
... based on wSedlacek's React 18 PR #89
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

2 participants