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 16 (fiber) #2996

Merged
merged 5 commits into from Sep 27, 2017
Merged

React 16 (fiber) #2996

merged 5 commits into from Sep 27, 2017

Conversation

kjs3
Copy link
Contributor

@kjs3 kjs3 commented Sep 26, 2017

This updates to React 16 and uses ReactDOM.hydrate() on the client as opposed to render().

Notes:

  • This does not include SSR streaming.
  • React no longer seems to be making charSet in meta tags lowercase. This made some tests fail and I played with manually doing this but that caused React to throw warnings. It's technically in spec so I guess it's fine.

@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 26, 2017

Might be good to keep #2659 in mind 👌 Can you add it as comment there?

@pruhstal
Copy link

@pruhstal pruhstal commented Sep 27, 2017

Thank you @kjs3!

@msand
Copy link

@msand msand commented Sep 27, 2017

Can this be installed from npm and used currently?

@kjs3
Copy link
Contributor Author

@kjs3 kjs3 commented Sep 27, 2017

@msand, I published it to npm under next-react-16 but that's a hassle to use. You need module-alias for the node side, an alias in webpack, and you need to change .babelrc to use next-react-16/babel instead of next/babel.

Instead of all that I just pushed up a branch so you can install from github. Change the next line in your package.json to this:

"next": "https://github.com/kjs3/next.js.git#next-react-16",

This next-react-16 branch is the same as the branch for this PR accept it includes dist/. I'll keep it up to date with any changes.

@kulshekhar
Copy link

@kulshekhar kulshekhar commented Sep 27, 2017

fwiw, I've tested this PR with a small app and it worked without issues 👍

Copy link
Member

@timneutkens timneutkens left a comment

Looks good! @arunoda has to review this too though, cause we need to decide what versioning we're going to use for this 👍

@kjs3
Copy link
Contributor Author

@kjs3 kjs3 commented Sep 27, 2017

@timneutkens: Yeah, I was wondering about that. I guess you could publish this under a tag like fiber if it needs some more vetting before going out under the next tag.

@timneutkens timneutkens merged commit d19cc97 into vercel:master Sep 27, 2017
3 checks passed
@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 27, 2017

Merged 👍

@kjs3 kjs3 deleted the react-16 branch Sep 27, 2017
@marbemac marbemac mentioned this pull request Sep 28, 2017
@arunoda
Copy link
Contributor

@arunoda arunoda commented Sep 28, 2017

Thanks @kjs3 and @timneutkens.
But I had to revert this for few reasons.

I'll continue this PR and do a release soon. (within few hours)

  1. We need to do this PR with backward compatibility. (Or simply do a major release) - Reason is we asked to users to use React 16 with this. If users forcefully go back to React 15 they lost dev time error tracking.

  2. We need to make sure it's better to use React. hydrate even for normal rendering.

arunoda added a commit that referenced this issue Sep 28, 2017
@arunoda arunoda mentioned this pull request Sep 28, 2017
arunoda added a commit that referenced this issue Sep 28, 2017
arunoda added a commit that referenced this issue Sep 28, 2017
* Revert "Keep some buffered pages, that won't be disposed. Fix #1939 (#2592)"

This reverts commit 3643612.

* Revert "Add beta installation instruction"

This reverts commit 418cc21.

* Revert "4.0.0-beta.1"

This reverts commit c2d98e2.

* Revert "Treat navigation to empty hash as hash navigate (#2971)"

This reverts commit c6bd6ef.

* Revert "React 16 (fiber) (#2996)"

This reverts commit d19cc97.
@kjs3
Copy link
Contributor Author

@kjs3 kjs3 commented Sep 28, 2017

@arunoda, you're right that this is a hard break to React 16. Are you wanting to maybe check for ReactReconciler and setup error handling with it if it exists?

As far as being better, SSR is more than 2x faster on the doorsteps.com codebase. I can't say I had problems with the client side render() when hydrating but hydrate() should be a little more flexible.

Let me know if there's specific work you'd like to see done?

@arunoda
Copy link
Contributor

@arunoda arunoda commented Sep 28, 2017

@kjs3 It's not a problem with this PR.
But we need to figure out a way to solve my point mentioned as 1) above or do a major release.

And this: https://twitter.com/arunoda/status/913442183243497473

@arunoda
Copy link
Contributor

@arunoda arunoda commented Sep 28, 2017

Actually I reverted my revert :D
Since @timneutkens already released a major version, we are good to go.

@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 28, 2017

👍 👍 👍 👌

@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 28, 2017

Talked about it with rauchg yesterday, major release is not a problem. Will be a beta for now 😄

@kjs3
Copy link
Contributor Author

@kjs3 kjs3 commented Sep 28, 2017

@arunoda: Ok cool. I’m following your tweet and will keep an eye out for an issue on the React repo about whether we should keep using hydrate for all renders client side.

@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 28, 2017

👌

@cloudle
Copy link

@cloudle cloudle commented Sep 28, 2017

@arunoda: I've tried, and our latest next.js npm still not happy with React 16 now

Using "next": "https://github.com/kjs3/next.js.git#next-react-16" .. can't wait to see this really merged to master.

@timneutkens thanks for your great work 👍

@kjs3
Copy link
Contributor Author

@kjs3 kjs3 commented Sep 28, 2017

@arunoda: 😬 I guess I shouldn't delete that branch then.

Please don't go to production with that. Or, if you do, fork it so I'm not the lynchpin in your setup. 😀

@cloudle
Copy link

@cloudle cloudle commented Sep 28, 2017

oops, @kjs3 thanks for it.. I'll keep sync and move back to next.js master as soon as it ready.

@timneutkens
Copy link
Member

@timneutkens timneutkens commented Sep 28, 2017

@cloudle you can just use next@beta at this very moment @cloudle 👍

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018
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.

None yet

7 participants