shirakaba / react-nativescript

React renderer for NativeScript
https://react-nativescript.netlify.com
MIT License
284 stars 14 forks source link

React 18 Support #89

Open wSedlacek opened 2 years ago

wSedlacek commented 2 years ago

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 commented 2 years ago

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:

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