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

Upgrade to Webpack 4 #8712 #8713

Merged
merged 6 commits into from
Mar 29, 2018
Merged

Conversation

crphang
Copy link
Contributor

@crphang crphang commented Mar 26, 2018

Fixes #8712

Outline of Solution

There is currently not many webpack dependencies other than UglifyJSPlugin that is affected. Upgraded webpack and installed webpack-cli (separated from webpack in webpack 4). Modified code to support UglifyJSPlugin.

Currently mode is set to production by default, which is similar to how we are doing things. This upgrade ensures that dependencies does not break and does not introduce new upgrades (will be part of upcoming issues and PRs).

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

@crphang Thanks for taking the time to look into this!

We will need to impose a newer Node.js version in the setting-up guide. Based on the release note of Webpack 4.0.0, Node 4 is no longer supported, and based on this tweet, Node 8.9.4+ is recommended. Also take a look at webpack/webpack#6579; I don't know how true/reliable it is.

Also, I hope this has been tested in AppVeyor or some other Windows-based environment?

@@ -25,6 +25,7 @@ const babel = {

module.exports = {
entry,
mode: 'production',
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed? Even if deployed, this remains a testing-only file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. It's not strictly necessary here. What it does is that it allows for various production environment only optimization, which includes minimizer (UglifyJS). But since we don't use it for test, we can leave it out for now.

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'll look into the node requirements and windows environment. Missed that since my local machine has a later version.

@wkurniawan07 wkurniawan07 self-assigned this Mar 26, 2018
@wkurniawan07 wkurniawan07 added the s.Ongoing The PR is being worked on by the author(s) label Mar 26, 2018
Copy link
Member

@tshradheya tshradheya left a comment

Choose a reason for hiding this comment

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

Tried and tested this on my windows setup and it works fine.

AppVeyor also working as expected https://ci.appveyor.com/project/tshradheya/teammates/build/1.0.22

package.json Outdated
@@ -10,7 +10,8 @@
"lintspaces-cli": "0.6.0",
"stylelint": "9.1.1",
"stylelint-config-standard": "18.2.0",
"webpack": "3.11.0"
"webpack": "4.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

@crphang Any reason why version 4.2?

When I did npm install webpack , it installed the 4.3.0 version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs currently is for 4.2 on Webpack official docs. I thought its good to follow that. Later versions of 4.x shouldnt have breaking changes, and can be upgraded iteratively. What do you guys think?

Copy link
Member

@tshradheya tshradheya Mar 28, 2018

Choose a reason for hiding this comment

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

Keeping 4.2 sounds fine to me in that case 👍

Copy link
Member

Choose a reason for hiding this comment

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

The docs currently is for 4.2 on Webpack official docs.

Are you sure? I just checked and it's already updated to 4.3.0 which was released only yesterday. I believe it's safe to use the latest version.
Clearly we can't be up-to-date all the time unless we use a non-exact versioning policy, but this shall be a discussion for another day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My folly, didnt check the most recent since my change was super recent as well. Will update it!

Thanks @tshradheya for the windows verification

Copy link
Member

@wkurniawan07 wkurniawan07 left a comment

Choose a reason for hiding this comment

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

LGTM, let's proceed to further improvements for Webpack

@wkurniawan07 wkurniawan07 added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 29, 2018
@crphang crphang changed the title Upgrade Webpack to Webpack 4 #8712 Upgrade to Webpack 4 #8712 Mar 29, 2018
@crphang crphang merged commit a524098 into TEAMMATES:master Mar 29, 2018
@whipermr5 whipermr5 added the e.1 label Mar 30, 2018
@whipermr5 whipermr5 added this to the V6.5.0 milestone Mar 30, 2018
@whipermr5 whipermr5 added the c.DevOps Process-related or build-related improvement and addition label Apr 14, 2018
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request May 14, 2018
jacoblipech pushed a commit to jacoblipech/teammates that referenced this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.DevOps Process-related or build-related improvement and addition s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants