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

Webpack 4 ✨ #3323

Merged
merged 4 commits into from
Oct 9, 2018
Merged

Webpack 4 ✨ #3323

merged 4 commits into from
Oct 9, 2018

Conversation

brandonc
Copy link
Contributor

I had some time last night to upgrade to Webpack 4. It's slightly faster, but doesn't offer any big advantages that I know of right now. If we migrate to babel 7, we can drop ts-loader since babel 7 can transpile typescript now. That could potentially knock 20s off our build time

Testing

npm start or npm run build-assets

Trade-offs

I don't know of any risks.

Dependencies

This branch is based on #3307

@brandonc brandonc changed the title Brandonc/webpack 4 Webpack 4 ✨ Sep 25, 2018
@brandonc
Copy link
Contributor Author

TIL you can't reopen a PR after force pushing it (#3310)

@nLight
Copy link
Contributor

nLight commented Sep 26, 2018

I'd like to hold merge this until we branch release/1.12

@nLight nLight removed the hold merge label Oct 5, 2018
"process.env.version": JSON.stringify(packageInfo.version)
}),
new UglifyJsPlugin({
Copy link
Contributor

Choose a reason for hiding this comment

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

Anything else to replace uglify?

@@ -15,6 +15,7 @@ delete dependencies["canvas-ui"];
delete dependencies["cnvs"];

module.exports = merge(common, {
mode: "production",
Copy link
Contributor

Choose a reason for hiding this comment

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

will this replace "process.env.NODE_ENV"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; webpack sets this and other behaviors like uglify automatically in this mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't intend for this to be a surprise but left the background in the commit messages . I'll be sure to add these to the PR description in the future https://webpack.js.org/concepts/mode/

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, my bad. Don't have a strong habit of reading commit messages during a review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you review my difficult PRs you will usually do better to review commit by commit. (Github makes this pretty easy with keyboard shortcuts BTW you can press 'n' and 'p' when looking at a pull/commit page). I usually take the time to groom my commits to make review easier to deal with.

@@ -65,6 +65,12 @@ module.exports = {
],
module: {
rules: [
{
type: "javascript/auto",
Copy link
Contributor

Choose a reason for hiding this comment

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

Would that transpile all the modules? Do we need this or it's more like a nice to have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a long term solution, but a workaround owing the to the fact that webpack 4 prefers mjs files to js files and react-apollo fails to account for this. apollographql/react-apollo#1737

@nLight nLight changed the base branch from brandonc/webpack_perf to master October 8, 2018 17:02
@nLight
Copy link
Contributor

nLight commented Oct 8, 2018

Conflicts!

@brandonc brandonc changed the base branch from master to brandonc/webpack_perf October 8, 2018 17:03
@brandonc
Copy link
Contributor Author

brandonc commented Oct 8, 2018

Ill merge #3307 and then rebase on master

@brandonc brandonc changed the base branch from brandonc/webpack_perf to master October 8, 2018 17:11
Copy link
Contributor

@GeorgiSTodorov GeorgiSTodorov left a comment

Choose a reason for hiding this comment

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

I don't think I know enough about webpack to approve or disapprove, but I tested it and I think it looks good. I like that you made separate commits with additional information.

@nLight
Copy link
Contributor

nLight commented Oct 9, 2018

I'd say the longer we wait the closer to a release we will be merging this :)

@GeorgiSTodorov
Copy link
Contributor

@nLight nLight merged commit 735e085 into master Oct 9, 2018
@nLight nLight deleted the brandonc/webpack_4 branch October 9, 2018 17:22
@mesosphere-ci
Copy link
Collaborator

🎉 This PR is included in version 2.27.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

5 participants