-
Notifications
You must be signed in to change notification settings - Fork 50
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
Cleanup server side renderer #1567
Conversation
This makes server part of webpack build way faster during development, ~10x. Benchmark (my xps): - OLD: 19.35s - NEW: 1.97s
|
||
export default render; | ||
const noSSR = (req, res) => res.send(pageRenderer()); | ||
export default (__DEV__ ? noSSR : require('./ssr.js').default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this doesn't import './ssr.js' in development, it doesn't have to compile all the millions of lines with js (since we don't use the SSR in dev, heh)
bc5ca8d
to
443d08d
Compare
app/render.js
Outdated
import routes from 'app/routes'; | ||
import type { Store } from 'app/types'; | ||
setConfig({ | ||
//pureSFC: true | ||
ignoreSFC: true, // RHL will be __completely__ disabled for SFC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe write what RHL and SFC stands for in the comment for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Almost forgot this..
Did some testing with this and https://github.com/mzgoddard/hard-source-webpack-plugin/. hard-source-webpack-plugin makes yarn start
take ~7-8.sec, and I had to tweak react-hot-loader to make it work. Not 110% stable tho, more like 60-70%, so we can look into that later.
From what I remember, we may as well just remove this setConfig altogether.
Some background info/kok
gaearon/react-hot-loader#1088 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yee. Works well without it, so I removed it now.
: '' | ||
} | ||
<script async src="https://js.stripe.com/v2/"></script> | ||
<script async src="https://js.stripe.com/v3/"></script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really related to this PR, but do we still need both stripe versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yee. :/
We still use the old checkout-api (v2) and the new react-stripe-elements
. We can remove v2 when this (not the GPay part) is done #1527. We can also use the new checkout api (just a few days old i think) https://stripe.com/docs/payments/checkout for users without google or apple pay. 😄
The plain Component(props) doesn't support hooks on ssr, so the prepare method will crash if a component use hooks. This doesn't matter as long as we don't use hooks before using prepare(). THen the warning can be ignored.
This makes server part of webpack build way faster during development,
~10x.
Benchmark (my xps):
Also:
react-with-hooks
for node. TherenderToString
does work, but no other low-level stuff :/