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

Warn about duplicate dependencies #2011

Closed

Conversation

darrenscerri
Copy link
Contributor

Add duplicate-package-checker-webpack-plugin as proposed here: #1844 .
Display compilation warnings on build.

duplicate packages

This feature can be quickly tested by creating an app, installing an old version of fbjs (yarn add fbjs@0.6.0) and importing fbjs from App.js. This will result in having duplicate fbjs packages since React already requires the most recent version of fbjs.

@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

"duplicate-package-checker" is not a very friendly way to log it. The end user doesn’t know what a "duplicate-package-checker" is. Instead, it should say:

The following packages are duplicated in the bundle:

and also add some instructions on how to avoid it.

@gaearon gaearon modified the milestones: 0.11.0, 0.10.0 May 15, 2017
@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

(I understand this may not be possible, but we need to work with its author to ensure it provides a good experience to our users. We also need to make sure we have some sensible instructions to follow to solve the issue.)

@darrenscerri
Copy link
Contributor Author

Been a while 😄 I've updated the plugin to display a better warning message and beefed up the Readme in the repo with some possible solutions for resolving duplicate package issues (https://github.com/darrenscerri/duplicate-package-checker-webpack-plugin)

As for the warning message, Webpack prepends all warnings with a "Warning in:" which is usually followed by a package name or a path to the file that the warning refers to, so I've settled down on starting the warning with the duplicated package name followed by relevant information.

CRA:
screen shot 2017-11-22 at 22 29 58

Non-CRA Webpack:
lodash

@Timer
Copy link
Contributor

Timer commented Nov 23, 2017

Fancy!

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

I think it's not reasonable to warn if the libraries depend on different major versions.
This can't be solved by deduping or any other method safely.

What do you think?

@darrenscerri
Copy link
Contributor Author

I understand your point, but I think that a different major version of a package is still something that the developer should at least be aware of, due to any of the following:

  • Your project is using an outdated package, which may no longer be supported
  • Your project is bloated by having 2 packages doing effectively the same thing
  • You might run into hard-to-debug issues if your project assumes only a single instance of the package

I agree that solving such issues in this case may not be easy, since it might require the developer to file an issue on the project to update the outdated dependency. Otherwise, the solution may be risky since their may be compatibility issues between the two versions of the package. Having said that, I don't think that giving such a heads-up to the developer is unreasonable.

The only downside I see right now is that using CRA, developers cannot easily exclude packages in order to dismiss the warning for a particular package if they acknowledge and are ok with having multiple versions of that package. The plugin itself supports exclusions, but only through a configuration object passed when instantiating the Webpack plugin.

@gaearon
Copy link
Contributor

gaearon commented Jan 10, 2018

How do you feel about adding an option to the plugin to only warn about same majors? I think we could take it in that case. (I agree with you in theory but in practice it will just mean everyone sees something like lodash there and applies incorrect workarounds.)

@@ -33,6 +33,7 @@
"css-loader": "0.28.7",
"dotenv": "4.0.0",
"dotenv-expand": "4.0.1",
"duplicate-package-checker-webpack-plugin": "^2.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please pin the version.

@darrenscerri
Copy link
Contributor Author

Yeah I agree, it might potentially lead to impulsive and unsafe workarounds if devs don't really understand the consequences.

@gaearon gaearon changed the base branch from master to next January 16, 2018 15:54
Copy link

@Luchanso Luchanso left a comment

Choose a reason for hiding this comment

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

Can I will review some PR from this repo? It will not confuse?

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

I think this is a blocker for the PR: darrenscerri/duplicate-package-checker-webpack-plugin#16

@gaearon
Copy link
Contributor

gaearon commented Jan 17, 2018

We should also make sure this doesn't fail the CI when duplicates are found.

@SavePointSam
Copy link

Is this still something planned for #3815? Based on the child issue discussion (darrenscerri/duplicate-package-checker-webpack-plugin#16 (comment)), this is no longer being considered. I would request the parent react-script@2.0 issue be updated if this functionality is being dropped.

If this is still desired, what needs to be done to push this forward? I see the fs performance issue is still open and rather stale, like this PR.

Personally, I think this would be a great addition. However, the concerns about actionability and clarity in what is happening will be very difficult to get right. Would it be worthwhile to have subdued output for advanced users that want to know this kind of things with a link to the User Guide that explains everything in greater detail? The biggest combatant to misinformation about solving issues like this, in my opinion, come from having a recommended set of information that is readily presented to the user. No need to search warning messages if there’s is a link right there saying learn more.

Ultimately, I suggest this is not a blocker for 2.0 as I don’t know if anyone clamoring for this functionality, though I totally see the merit behind it. Maybe it’s wortwhile to put in a 2.X?

Thanks! ^_^

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@Timer Timer modified the milestones: 2.0.x, 2.x Sep 26, 2018
@gaearon gaearon closed this Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants