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

Handle react-dom/server minified version #1862

Merged
merged 1 commit into from
May 3, 2017

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented May 3, 2017

Ref: #1855

That's simply because it has no effect since we don't run webpack on the server.
And for the server, file size difference doesn't matter a lot.

That's simply because it has no effect since we don't run webpack on the server.
And for the server, file size difference doesn't matter a lot.
@@ -299,10 +299,11 @@ export default async function createCompiler (dir, { dev = false, quiet = false,
}

if (!dev) {
// We do this to use the minified version of React in production.
// This will significant file size redution when turned off uglifyjs.
Copy link
Contributor

Choose a reason for hiding this comment

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

How can you turn off UglifyJS? removing it manually from webpack config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

webpackConfig.resolve.alias = {
'react': require.resolve('react/dist/react.min.js'),
'react-dom': require.resolve('react-dom/dist/react-dom.min.js'),
'react-dom/server': require.resolve('react-dom/dist/react-dom-server.min.js')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be kept. It's a different entry point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by different entry point? We don't use webpack in server. So, there's no use in that.
Isn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by different entry point?

react-dom and react-dom/server are different modules. Can verify by the import declarations themselves and by the react-dom/dist/* directory contents.

So, there's no use in that. Isn't it?

There is. It's same reasons to use prod-ready vs dev-mode React in the browser. ReactDOM and react-dom/server still has lot of dev-specific code blocks that should be/are properly removed in the minified releases.

The import here will be pulling the dev-mode of react-dom/server without this alias. Although Next.js injects a NODE_ENV and uglifies, it doesn't do everything the React team does. (Only a subset of prod-build config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I get it. But it's not belong here as we don't use webpack on the server. We use our own module loading system on the server.
So, we need to do it there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, derp. 🙄 😆 For some reason I was under the impression that ReactDOM was embedded into that file.... I must have needed coffee at the time.

In that case, adding this to server/build/babel/preset.js should be it, right?

'react-dom/server': relativeResolve('react-dom/dist/react-dom-server.min.js'),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Got this error: TypeError: Cannot read property 'ReactCurrentOwner' of undefined.
Looking at some links and lets see.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume we need to use this: https://www.npmjs.com/package/module-alias

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.. how are you getting that error? I just pulled a fresh master, added that change, built, then copied to examples/hello-world once again. Built then ran that demo & works without errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check this: facebook/react#8788

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Sorry, didn't realize Next.js stopping running all tests before commits.

Seems like React team is aware of the issue; it's in multiple places but some of the tickets have a Won't fix label on them. I checked 15.4.0 to 15.5.4 and the error exists in all of them.

Would you be open to compiling a server bundle (as part of next build)? This way you can apply all the proper optimizations ahead of time... and might even be able to remove Babel regenerator/runtime which would be huge!

@lukeed
Copy link
Contributor

lukeed commented May 3, 2017

Production React makes a big difference in the server. See this comparison and this thread for info.

@arunoda arunoda changed the title React react-dom/server minified alias. Handle react-dom/server minified version May 3, 2017
@arunoda
Copy link
Contributor Author

arunoda commented May 3, 2017

As mentioned in this comment (facebook/react#8788 (comment)) it's impossible to load the minified version in the server side. (Specially we don't use webpack in the server)

So, we need to wait for the next version of React which fixes this issue (or the root slowness)
Anyway, I am taking this PR.

@arunoda arunoda merged commit ee9dba9 into vercel:master May 3, 2017
@arunoda arunoda deleted the improve-react-min-js-support branch May 3, 2017 20:59
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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.

3 participants