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

WIP: Using React.createContext instead of ChildContext #5935

Closed
wants to merge 5 commits into from
Closed

WIP: Using React.createContext instead of ChildContext #5935

wants to merge 5 commits into from

Conversation

chrislloyd
Copy link
Contributor

@chrislloyd chrislloyd commented Dec 23, 2018

This is in respose to #5716. I'm tackling some of the smaller instances first just to get a sense of the project, coding style, tests etc.

It seemed like there was a little discussion about whether this change would alienate Preact and Inferno users however that's a decision for somebody else to make - I for one would prefer to take advantage of the latest and greatest React features.

The test plan is pretty simple - it's a refactoring so existing tests shouldn't break. Also completion milestone is removing prop-types as a dependency.

TODO

  • headManager
  • loadable
  • router
  • _documentProps

Note: I'm going to ignore the usage of legacy contextTypes in the examples as they don't add directly to Next's bundle size and I'm sure that each example library will have different best practices on how to update to React 17.

<NextScript />
</body>
</html>
return <DocumentPropsContext.Provider value={this.props}>
Copy link
Member

@timneutkens timneutkens Dec 23, 2018

Choose a reason for hiding this comment

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

This is a breaking change, as users implement render() in a custom document: https://github.com/zeit/next.js#custom-document

Copy link
Member

Choose a reason for hiding this comment

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

The best way to do this is adding the provider to renderDocument in packages/next-server/server/render.tsx

@timneutkens
Copy link
Member

I think the best way to go about moving this is gradually changing every component over, that makes sure the scope of the PR is smaller (targeted at a single feature) and makes it easier to see where the tests fail 👍

@chrislloyd
Copy link
Contributor Author

Great yeah - seeing as headManager and documentProps are quite simple will keep this together (unless you feel strongly) but happy to create separate PRs for both loadable and router (which I suspect will be more involved). Thanks for the review!

@chrislloyd
Copy link
Contributor Author

Also, is there a way to run the integration tests easily locally? They've been grinding my laptop to a halt because they spawn a tonne of Chrome processes.

@timneutkens
Copy link
Member

seeing as headManager and documentProps are quite simple will keep this together (unless you feel strongly)

I'd like to split them for the reason that we can then compare bundle size / if everything is bundled correctly

@timneutkens
Copy link
Member

Also, is there a way to run the integration tests easily locally? They've been grinding my laptop to a halt because they spawn a tonne of Chrome processes.

You can run individual tests:
https://github.com/zeit/next.js/blob/canary/contributing.md#to-run-tests

@chrislloyd
Copy link
Contributor Author

Closing in favor of #5945. Again, thanks for the review @timneutkens!

@chrislloyd chrislloyd closed this Dec 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants