BRS 7 - Full SSR Hard Source and Loadable support#77
BRS 7 - Full SSR Hard Source and Loadable support#77Andrew Jones (ajones513) wants to merge 27 commits intoSkyscanner:brs-v7.0.5from
Conversation
* (Temporarily) added hard-source plugin * Write loadable-stats to disk for consumption by server
…isation, definePlugin typeof window
Fix iconv loader
Ollie Curtis (olliecurtis)
left a comment
Ollie Curtis (olliecurtis)
left a comment
There was a problem hiding this comment.
Looks good but have a few clarifying questions that need answering just for future maintaining of this especially in the .ssr.js file as this is just a copy of the webpack.config.js file but modified for SSR.
| // makes for a smoother build process. | ||
| const shouldInlineRuntimeChunk = process.env.INLINE_RUNTIME_CHUNK !== 'false'; | ||
| // We might not want to use the hard source plugin on environments that won't persist the cache for later | ||
| const useHardSourceWebpackPlugin = |
There was a problem hiding this comment.
Rather than an env variable this might be better as a config value instead?
There was a problem hiding this comment.
As it's experimental (mainly because mzgoddard/hard-source-webpack-plugin#419 + waiting for Webpack 5), I've found value in flipping it on/off at will - so was thinking the environment variables maybe gave more flexibility.
If there's cleaner way without needing to make a code change in the consuming project that would still be good.
| // makes for a smoother build process. | ||
| // const shouldInlineRuntimeChunk = process.env.INLINE_RUNTIME_CHUNK !== 'false'; | ||
| // We might not want to use the hard source plugin on environments that won't persist the cache for later | ||
| const useHardSourceWebpackPlugin = |
| // Some apps do not use client-side routing with pushState. | ||
| // For these, "homepage" can be set to "." to enable relative asset paths. | ||
| const shouldUseRelativeAssetPaths = publicPath === './'; | ||
| // const shouldUseRelativeAssetPaths = publicPath === './'; |
There was a problem hiding this comment.
Could you confirm why this has been removed?
There was a problem hiding this comment.
It was only used by MiniCssExtractPlugin
| shouldUseRelativeAssetPaths ? { publicPath: '../../' } : undefined | ||
| ), | ||
| }, | ||
| // isEnvDevelopment && require.resolve('style-loader'), |
There was a problem hiding this comment.
Could you confirm why this has been removed?
There was a problem hiding this comment.
In standard CRA, in development, styles are served client-side only, via the Webpack dev server. The CSS is served inside the .js assets.
I didn't want to get into changing that - so in development, SSR should just ignore styles completely.
| new webpack.DefinePlugin(env.stringified), | ||
| new webpack.DefinePlugin({ | ||
| ...env.stringified, | ||
| 'typeof window': '"undefined"', |
There was a problem hiding this comment.
Could you confirm what this is doing?
There was a problem hiding this comment.
https://webpack.js.org/plugins/define-plugin/#usage - defining typeof window is the closest I've seen to a 'standard' way of stripping out client/server-only code from their respective bundles.
| }), | ||
| // This is necessary to emit hot updates (currently CSS only): | ||
| isEnvDevelopment && new webpack.HotModuleReplacementPlugin(), | ||
| // isEnvDevelopment && new webpack.HotModuleReplacementPlugin(), |
There was a problem hiding this comment.
Could you confirm why this has been removed?
There was a problem hiding this comment.
As above re: styles, and I don't think https://webpack.js.org/concepts/hot-module-replacement/ is designed for the server.
| filename: 'ssr.css', | ||
| // chunkFilename: 'static/css/[name].[contenthash:8].chunk.css', | ||
| }), | ||
| // isEnvProduction && |
There was a problem hiding this comment.
Could you confirm why this has been removed? Guessing it relates to the HardSourcePlugin?
There was a problem hiding this comment.
Just the styles again.
Adding new ignore option
Ollie Curtis (olliecurtis)
changed the title
Ollie Curtis (olliecurtis)
force-pushed
the
custom
branch
from
d510ff8 to
19acdf8
Compare
Ollie Curtis (olliecurtis)
force-pushed
the
custom
branch
from
19acdf8 to
6d0ee70
Compare
Ollie Curtis (olliecurtis)
changed the title
|
Closing this PR as v7 is no longer supported version at Skyscanner - these changes have been ported to newer versions and also improved in #95 |
2 participants
See these PRs for full description: