-
Notifications
You must be signed in to change notification settings - Fork 1
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
Task2 webpack #2
Conversation
config/webpack.config.common.ts
Outdated
entry: resolve("src", "index.tsx"), | ||
output: { | ||
path: resolve("build"), | ||
filename: "[name].[chunkhash].bundle.js", |
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.
Calculating content hash is relatively expensive and there's no point doing that in development mode, so, probably, this particular part should be specific to production mode only.
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.
Okay, will move it to prod build
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.
Done
template: join("src", "index.html"), | ||
}), | ||
], | ||
optimization: { |
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.
That would, probably, make more sense for production mode only.
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.
I don't agree here. Having a separate vendor chunk is useful in dev mode as well. It's better to keep the two modes as similar as possible to avoid prod bugs which not be reproduced on dev mode.
Also, it's a generally accepted practice to have a separate vendor chunk rather then build everything in one file
package.json
Outdated
"webpack-merge": "^5.8.0" | ||
}, | ||
"scripts": { | ||
"start": "cross-env NODE_ENV=dev webpack server", |
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.
You should bear in mind that cross-env
is no longer developed, which may be the reason of its declined popularity, so you may opt for corresponding command-line parameter, consider alternative solution or expose webpack config as a function and pass current mode as a command line parameter.
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.
No problem here. Should work fine for this project's life-time 🙂
Thanks for the notification. Didn't know it's deprecated.
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.
Specified webpack configuration instead.
Probably need to update HW description to use webpack env flag or specify config file in the script
Have DEV and PROD build commands (use env variables)
|
||
ReactDOM.render( | ||
<React.StrictMode> | ||
<GlobalStyles /> |
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.
You may, possibly, want to wrap your <App />
into ThemeContext.Provider
to share some common styling parameters (palette, font settings, etc) across your entire app.
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.
Will look into that in the scope of the next HW since I didn't work with styled-components before. Thanks
<GlobalStyles /> | ||
<App /> | ||
</React.StrictMode>, | ||
document.querySelector("#root") |
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.
querySelector()
is drastically slower than more conventional getElementById()
. It won't, probably, hurt performance of the entire app in your particular case (since used only once), however, the latter looks much more comprehensive, while the former is better suited for more sophisticated queries.
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.
Probably slower. But it's micro-optimization in this case, that's not an issue.
Prefer readability over performance and optimize only when needed.
For me, who worked with jQuery querySelector is more intuitive to use.
No description provided.