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

chore: upgrade all dependencies, devDependencies #290

Merged
merged 11 commits into from
Mar 1, 2018

Conversation

dhruvdutt
Copy link
Member

@dhruvdutt dhruvdutt commented Feb 26, 2018

What kind of change does this PR introduce?
Feature/Upgrade

Dependency Status Dependency Status

Did you add tests for your changes?
Updated migrate snapshot (031b798)

Summary

  • Upgrades all dependencies, devDependencies to latest except yargs*. 🚀
  • Update supports-color usage to resolve breaking changes
  • Remove unused devDependencies: flow-bin, flow-remove-types
  • Drops support for Node.js v4, going forward we only support v6+ (same as webpack-dev-server >= 3.0.0 and webpack >= 4.0.0)
    • Other dependencies that dropped Node.js v4 support:

*We will need to doublecheck on yargs changelog for breaking changes in version 10.0.0 and 11.0.0

Does this PR introduce a breaking change?
No

package.json Outdated
"eslint-plugin-node": "^5.1.0",
"flow-bin": "^0.49.1",
"eslint-plugin-node": "^6.0.1",
"flow-bin": "^0.66.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove flow-bin, it's not used anymore

@ematipico
Copy link
Contributor

I think you should rebase from master. We removed the job that was causing this problem with the CI. (https://github.com/webpack/webpack-cli/pull/277/files#r170428216)

package.json Outdated
@@ -123,7 +123,6 @@
"cz-customizable": "^5.2.0",
"eslint": "^4.2.0",
"eslint-plugin-node": "^6.0.1",
"flow-bin": "^0.66.0",
"flow-remove-types": "^1.2.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to bother again! Just noticed that we can also remove flow-remove-types!

Copy link
Member Author

@dhruvdutt dhruvdutt Feb 26, 2018

Choose a reason for hiding this comment

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

Ah! No worries. I just ran npm-check to check for all unused deps and this is the list of unused deps returned in output:

Can you please confirm which ones to keep/remove?

global
recast
uglifyjs-webpack-plugin
@commitlint/cli
@commitlint/prompt-cli
commitizen
conventional-changelog-lint-config-cz
husky
lint-staged
schema-utils

Copy link
Member

Choose a reason for hiding this comment

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

No one here

Copy link
Member Author

@dhruvdutt dhruvdutt Feb 26, 2018

Choose a reason for hiding this comment

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

I've cleaned flow-bin, flow-remove-types as suggested by @ematipico
Also, updated PR description.

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Gotta double check the colors thing, do not update yargs, as there's a security volun on the new version.

@dhruvdutt
Copy link
Member Author

dhruvdutt commented Feb 26, 2018

@ev1stensberg Reverted back to "yargs": "9.0.1" and added a note about it in PR description.

@dhruvdutt dhruvdutt changed the title Upgrade all dependencies, devDependencies chore: upgrade all dependencies, devDependencies Feb 26, 2018
@dhruvdutt dhruvdutt force-pushed the upgrade/deps branch 3 times, most recently from 31706f4 to fbc645d Compare February 27, 2018 10:36
bin/webpack.js Outdated
@@ -318,7 +318,7 @@
});

if (typeof outputOptions.colors === "undefined")
outputOptions.colors = require("supports-color");
outputOptions.colors = require("supports-color").stdout;
Copy link
Member

Choose a reason for hiding this comment

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

Anything changed in a newer version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't look like anything changed relevant to us other than the API.
There are no changelogs or release notes but you can take a look at this PR #64.

Copy link
Member

Choose a reason for hiding this comment

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

Remove the stdout and you're good. Doesn't work with it

bin/webpack.js Outdated
@@ -318,7 +318,7 @@
});

if (typeof outputOptions.colors === "undefined")
outputOptions.colors = require("supports-color");
outputOptions.colors = require("supports-color").stdout;
Copy link
Member

Choose a reason for hiding this comment

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

Remove the stdout and you're good. Doesn't work with it

@webpack-bot
Copy link

@dhruvdutt Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ev1stensberg Please review the new changes.

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

Need another reviewer to lgtm this, it's a big change, might break some parts we don't test

@evenstensberg
Copy link
Member

rebase against master

@webpack-bot
Copy link

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@dhruvdutt
Copy link
Member Author

Already rebased.

All tests basically fail with this error: (Check expected and contain values)

image

It should match but it doesn't. Most likely there's some weird issue that ANSI terminal color codes aren't parsed due to some reason when the tests run. Check the screenshot below. Printing stdout like:

console.log(stdout[5]);
console.log(stdout);

image

However this doesn't happen when we add stdout in supports-color usage.

require("supports-color").stdout

image

There are no changelogs for supports-color versions but we can go through the latest docs.

Currently added stdout back to supports-color.
LMK if we have some other fix or missing something.

@evenstensberg
Copy link
Member

Conflicts

@dhruvdutt
Copy link
Member Author

@ev1stensberg Resolved. 💯

@@ -37,7 +37,7 @@ module.exports = function processOptions(yargs, argv) {
});

if (typeof outputOptions.colors === "undefined")
outputOptions.colors = require("supports-color");
outputOptions.colors = require("supports-color").stdout;
Copy link
Member

Choose a reason for hiding this comment

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

No stdout here

bin/webpack.js Outdated
@@ -318,7 +318,7 @@
});

if (typeof outputOptions.colors === "undefined")
outputOptions.colors = require("supports-color");
outputOptions.colors = require("supports-color").stdout;
Copy link
Member

Choose a reason for hiding this comment

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

No stdout here

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing this breaks the tests. Did you read my above comment?
Do we want the tests to fail?

Copy link
Member

Choose a reason for hiding this comment

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

I want colors to work as intended. If the tests are failing you gotta fix em 👩‍💻✌️

Copy link
Member Author

@dhruvdutt dhruvdutt Mar 1, 2018

Choose a reason for hiding this comment

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

Coolio. So, in that case we can use something like ansi-parser to parse stdout inside tests. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Remove the stdout and lemme have a look

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. You're up captain. 💂‍♂️

bin/webpack.js Outdated
@@ -80,7 +80,7 @@
type: "boolean",
alias: "colors",
default: function supportsColor() {
return require("supports-color");
return require("supports-color").stdout;
Copy link
Member

Choose a reason for hiding this comment

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

no stdout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants