-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update webpack setup to improve dev and debugging #11
Conversation
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.
Generally looks good! Just a few questions
|
||
module.exports = { | ||
resolve: { | ||
modules: [ 'node_modules' ], |
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 think this is what resolve
defaults to, so you shouldn't need to specify this.
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.
A couple of iterations ago I had it automatically looking for modules in us-forms-system
subdirectories as well but took it out because I wasn't sure if it was helping or not. I left this in as a reminder but we can take it out.
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.
gotcha, in that case if you want to leave it in that's fine!
module.exports = { | ||
resolve: { | ||
modules: [ 'node_modules' ], | ||
extensions: ['.js', '.jsx'] |
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.
This may still be needed in order to resolve the .jsx
extension, which I don't believe is included by 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.
Yeah i don't think jsx is recognized by default.
const UglifyJSPlugin = require('uglifyjs-webpack-plugin'); | ||
const BundleAnalyzerPlugin = require('webpack-bundle-analyzer').BundleAnalyzerPlugin; | ||
|
||
module.exports = merge(require('./webpack.common.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.
Did this production webpack config come from somewhere? Curious for the reasoning behind some of the additions here.
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.
The stats were basically to just show what was available, we can turn it down a bit if the console seems too noisy. The chunks are a work in progress, the size of the app is really too large for a single bundle but I'm not sure what we can do to make that more automatic for someone using the starter app. For the BundleAnalyzerPlugin I am having it create a report in an html file but not automatically displaying it.
@annekainicUSDS did you want any changes before this lands? |
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.
LGTM!
Fixes #5
Fixes #10
npm serve
Since this is an app (and not a lib) I assume we don't want to commit build artifacts in
/public
, at least for now. We can always revisit, and we probably will once we settle on some solution for usds/us-forms-system#66 .For the serving of production files I used
npx
which will install thehttp-server
package on demand if it isn't present. Alternately I can add it todevDependencies
and it can always be installed. I don't have a preference either way.If this looks good I can add a commit and update the README.md to describe these features.