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

refactor: renderer update #2360

Merged

Conversation

bart-krakowski
Copy link
Contributor

This PR contains refactor of the renderer, which means:

  • Passes all types to type parameters to Reconciler. It simplified the API and will avoid potential typing problems.
  • React-reconciler has some additional required properties, which we have missed (supportsPersistence and supportsHydration)
  • Removes some ts-ignore, which makes the API a little bit more bulletproof.
  • Fixes some linting issues and typos ;)

Btw. I'm currently learning Fiber and Reconciler and I'm so grateful because this is a superb playground and benchmark how it should work! Thanks so much! 🎉

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 7, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ffbef7b:

Sandbox Source
example Configuration

Copy link
Member

@CodyJasonBennett CodyJasonBennett left a comment

Choose a reason for hiding this comment

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

I'm AFK for the week right now, but I'd have to cc @drcmda as far as changes. It would be good to test in an SSR environment (namely Nextjs) to double-check that we're implementing timeout fallbacks right.

@CodyJasonBennett CodyJasonBennett requested a review from drcmda July 8, 2022 10:48
@joshuaellis
Copy link
Member

I agree. We need to test this in an SSR env before it's merged.

@drcmda
Copy link
Member

drcmda commented Jul 11, 2022

this looks good! can anyone test nextjs and see if it's working? from looking at the code, it seems it's not changing timing functions? @bart-krakowski

@CodyJasonBennett
Copy link
Member

Looks good, can confirm that this works in Next/Node and through different scheduler scenarios with suspense and startTransition. I'll wait to add related coverage until v9 (some upstream stuff to sort out), but I'll get this in. Thanks!

@CodyJasonBennett CodyJasonBennett merged commit c12fec4 into pmndrs:master Jul 14, 2022
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.

4 participants