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

Add support for Webpack 4 #17

Merged
merged 19 commits into from
Mar 20, 2018
Merged

Conversation

christiango
Copy link
Contributor

When users of the Duplicate Package Checker plugin upgrade to webpack 4, they will see a warning like this:

(node:11200) DeprecationWarning: Tapable.plugin is deprecated. Use new API on .hooks instead

This PR updates to use the new tapable interface, which causes the warning to go away. I validated that this works by using the node debugger on an existing project and making sure that the code for the plugin is executed and that the modules are still properly iterated over.

I also added a dev-dependency for prettier so that the precommit hook doesn't fail if you don't have prettier installed globally.

This is incompatible with Webpack <4, so this should probably be versioned as a new major release.

@christiango
Copy link
Contributor Author

Looking into the test failures.

@christiango
Copy link
Contributor Author

So it appears to be the case that the tests are failing because the order in which things are appearing in the snapshots are now different. Do we want to preserve the ordering from webpack v3 or just update the snapshots?

@@ -0,0 +1,13 @@
{
"version": "0.2.0",
"configurations": [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets you debug the unit tests in vscode, I found it useful

@@ -17,7 +17,7 @@ function DuplicatePackageCheckerPlugin(options) {
}

function cleanPath(path) {
return path.split("/node_modules/").join("/~/");
return path.split(/[\/\\]node_modules[\/\\]/).join("/~/");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes clean path work in windows


sortedDuplicateKeys.map(name => {
let instances = duplicates[name].sort(
(a, b) => (a.version < b.version ? -1 : 1)
Copy link
Contributor Author

@christiango christiango Mar 9, 2018

Choose a reason for hiding this comment

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

Not perfect since it is just a string sort, but it keeps the snapshot tests from being fragile against the ordering of the duplicates and the duplication instances

@christiango
Copy link
Contributor Author

Hey @darrenscerri will you have a chance to take a look at this soon?

@darrenscerri
Copy link
Owner

Hey @christiango! Great work and thanks so much for this PR! Two small nits:

  • Can we also add a sanity test with mode: "production" in Webpack? You can copy/reuse the simple test.
  • Let's kill node@4 as a testing target in Travis, Webpack@4 only supports node>=6.11.5 anyway

@christiango
Copy link
Contributor Author

I was having issues in the CI environment earlier on with production mode. In particular, something wasn't working with regards to uglify. I'll give it another shot

I'll push the update to the travis yml file.

@christiango
Copy link
Contributor Author

Let me try upgrading Jest, this PR looks promising for the uglify error: jestjs/jest#4622

@christiango
Copy link
Contributor Author

@darrenscerri it seems like uglify isn't working in the CI. I've added a production sanity test without uglify and that works.

Let me know if there is anything else you want me to change.

@darrenscerri
Copy link
Owner

That's quite weird, and it's only failing on 1 test. I'll look into it.

@christiango
Copy link
Contributor Author

@darrenscerri it only fails on 1 test because that's the only one that has production mode set in webpack. Since this is limited to Uglify, I worked around it by disabling minification in the test, which is consistent with the existing tests written for webpack <=3. Do you think there's value in having minification run during the UT's?

@darrenscerri
Copy link
Owner

darrenscerri commented Mar 20, 2018

Hey @christiango. I've done some investigation and it appears that the problem with the tests is twofold:

  1. Tests run in parallel by default. Travis sandboxes are resource-limited machines, and jest would spin up all the tests to run in parallel, each running Uglify, which is quite CPU intensive. The Timeout - Async callback was not invoked within the 5000ms timeout specified by jest.setTimeout in this case means that the test simply didn't finish within a default timeout of 5 seconds and needs more time. Increasing the timeout would have solved the issue, but there's a better solution for resource-limited machines: run the tests sequentially.

  2. The TypeError: setTimeout(...).unref is not a function is because Jest runs with "jsdom" as a test environment by default. Changing this to "node" fixes the issue, since native timers in node have an unref function, while jsdom timers apparently don't.

@christiango
Copy link
Contributor Author

Thanks for investigating! That makes a lot of sense.

@darrenscerri darrenscerri merged commit f78729d into darrenscerri:master Mar 20, 2018
@darrenscerri
Copy link
Owner

Thanks for the PR @christiango! 🚀

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.

2 participants