darrenscerri / duplicate-package-checker-webpack-plugin

🕵️ Webpack plugin that warns you when a build contains multiple versions of the same package
MIT License
709 stars 29 forks source link

Add support for Webpack 4 #17

Closed christiango closed 6 years ago

christiango commented 6 years ago

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.hooksinstead

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 commented 6 years ago

Looking into the test failures.

christiango commented 6 years ago

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?

christiango commented 6 years ago

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

darrenscerri commented 6 years ago

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

christiango commented 6 years ago

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 commented 6 years ago

Let me try upgrading Jest, this PR looks promising for the uglify error: https://github.com/facebook/jest/pull/4622

christiango commented 6 years ago

@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 commented 6 years ago

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

christiango commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

Thanks for investigating! That makes a lot of sense.

darrenscerri commented 6 years ago

Thanks for the PR @christiango! 🚀