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

Compiled out process.env.NODE_ENV for Rax and React benchmarks #10

Closed
wants to merge 1 commit into from

Conversation

aickin
Copy link

@aickin aickin commented Jan 16, 2017

This is a new version of #5, built on top of #7. It compiles out process.env.NODE_ENV in both the Rax and React benchmarks, in both the renderToString and server versions. It cannot compile out process.env.NODE_ENV in Vue because Vue's server renderer is not webpack-compatible.

For more background, read #5, #6, and #7.

On my machine, before this PR, I get:

-----------compare renderToString----------
Rax#renderToString x 93.54 ops/sec ±3.63% (65 runs sampled)
React#renderToString x 58.27 ops/sec ±1.49% (58 runs sampled)
Vue#renderToString x 205 ops/sec ±4.16% (72 runs sampled)
Fastest is Vue#renderToString

And after:

-----------compare renderToString----------
Rax#renderToString x 93.09 ops/sec ±4.20% (58 runs sampled)
React#renderToString x 140 ops/sec ±1.29% (74 runs sampled)
Vue#renderToString x 207 ops/sec ±4.22% (72 runs sampled)
Fastest is Vue#renderToString

@imsobear
Copy link
Collaborator

@aickin Could we avoid compile server-side code to assets? Under normal circumstances, the server-side code is not required compile.

@imsobear
Copy link
Collaborator

And in real project, I think most people won't use this directory specification to works.

@aickin
Copy link
Author

aickin commented Jan 16, 2017

@imsobear I disagree here. The right way to deploy React SSR to production is to compile out process.env.NODE_ENV on both server-side and client-side code. And to be clear, you were compiling the app code for server-side already, right?

@imsobear
Copy link
Collaborator

About SSR, my ideal way is that don't change the code way both in server-end and front-end. But the truth is front-end must do some change, we can't avoid that. And now for this problem, we need compile the routes or controllers code to assets. I think it is strange and ugly as a Noder.

Maybe there is a better way to solve this problem. :)

@imsobear
Copy link
Collaborator

We preferentially try to solve the renderToString benchmark problem. And ignore the qps comparition temporarily.

@aickin
Copy link
Author

aickin commented Jan 16, 2017

OK, so you want me to stop compilation of the controllers and remove the qps benchmark?

@imsobear
Copy link
Collaborator

Yes, don't compile the controllers and ignore qps benchmark temporarily. And we just solve the problem that how to require('react-dom/dist/react-dom-server.min') in benchmarks/renderToString.js.

@aickin
Copy link
Author

aickin commented Jan 16, 2017

we just solve the problem that how to require('react-dom/dist/react-dom-server.min') in benchmarks/renderToString.js

I'm pretty sure this won't work (and I know it isn't a supported way to use the library). Didn't you try it already and have problems?

@imsobear
Copy link
Collaborator

Yeah, It can't works. In my opinion, if this way could greatly improve React SSR performance. React should support this ability or a best practice. I don't agree with use many hacks for this feature.

Thanks for your guidance. And I have learned a lot from your video. 😅

@yyx990803
Copy link
Contributor

yyx990803 commented Jan 17, 2017

I tend to agree that "compiling server side code with webpack to remove access to process" is not what Node developers would typically do, and likely many React users are not even aware of this possibility. So the current results are probably more representative of React SSR perf in the wild. However, it can also be a bit unfair to not show React's perf with full optimization.

Maybe we can add this optimization as a separate entry in the benchmarks? (preserve /react, add /react-optimized).

P.S. I've seen on Twitter that the React team is considering exporting a pre-compiled bundle for ReactDOMServer if process.env.NODE_ENV === 'production', which should eliminate the need for this workaround in the future.

@imsobear imsobear closed this Mar 12, 2017
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.

3 participants