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

updates to webpack4 #77

Merged
merged 2 commits into from
Dec 12, 2018
Merged

updates to webpack4 #77

merged 2 commits into from
Dec 12, 2018

Conversation

christopher-johnson
Copy link
Contributor

@christopher-johnson christopher-johnson commented Dec 10, 2018

closes #76

  • also updates babel and eslint packages to work with new webpack4 api
  • babel-preset-react-app includes both preset-env and preset-react (among others) as a bundle.
  • mixing import and module.exports is no longer allowed in an ES6 module. (ref changes in index.js)
  • includes required linting changes to pass build and tests. Most are react/destructuring-assignment
  • adds "no-console": "off" rule to .eslintrc as console log statements are typical with node.
  • adds paths.js which includes path resolvers and aliases for scripts.
  • adds NODE_ENV to script defs in package.json.

includes required linting changes to pass build and tests
Copy link
Collaborator

@mejackreed mejackreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @christopher-johnson ! This looks great to me. I really like how you list out the changes individually in the PR.

One quick question, should the development build (running npm start) get the development environment?

"start": "NODE_ENV=development npm run build && concurrently \"npm run build:watch\" \"npm run server -- -p 4444\""
},
"eslintConfig": {
"extends": "react-app"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this work with the .eslintrc file which extends airbnb. Should that just be an array and specified there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint will read both config from .eslintrc and the package.json. I agree that it would be cleaner to put that in the .eslintrc.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Do you mind making that small change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it turns out that eslint does not read the package.json if there is an .eslintrc. if it had, then eslint-config-react-app (as well as eslint-plugin-flowtype) would have been required as a dependencies. this is a useful extension for accessibility checks, so I do not think that there is a downside to adding it here. note, however, that it is entirely optional.

@christopher-johnson
Copy link
Contributor Author

about the dev mode and dev server in the start script, this still needs some work. It is fairly typical to have a separate webpack config for development and the dev server, and this can be done in another PR.

currently, the start script has an extra NODE_ENV=development that does nothing in the initial build since it is overridden in the build script with a production mode, and this should be removed. But the watch script is set to the development environment. This should disable webpack from compiling production optimizations in concurrency.

adds mode to build scripts
Copy link
Collaborator

@mejackreed mejackreed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for breaking this out @christopher-johnson. We reviewed this today in the POC session and appreciate your work on upgrading to webpack 4. We did see a few things that we would like to update, so will likely tackle that as a team in our pairing session.

Thanks again! 🥂

@mejackreed mejackreed merged commit a563b46 into ProjectMirador:master Dec 12, 2018
@christopher-johnson christopher-johnson deleted the webpack4 branch January 4, 2019 10:14
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.

update to webpack 4
2 participants