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

check.js: Ignore dedup warnings for bundled dependencies #3337

Merged
merged 1 commit into from
May 19, 2017

Conversation

rufman
Copy link
Contributor

@rufman rufman commented May 6, 2017

Any package with bundledDependencies will be added to a hash of bundled
deps, which are used to check against when printing dedup warnings.
This is because bundled dependencies could result in duplications which
we would otherwise detect as false positives.

Fixes #3299

Summary
yarn check shouldn't return dedup warnings for any bundled dependencies, such as in the case of nyc.

Test plan
A simple package.json with just nyc, will return 41 warnings for packages present in nyc's packageJson.bundledDependencies when doing yarn check on the current master. When doing yarn check on this commit, no warnings are returned, since the dedup warnings resulting from nyc's bundled dependencies are ignored.

@rufman rufman closed this May 7, 2017
@rufman rufman reopened this May 7, 2017
@rufman rufman closed this May 8, 2017
@rufman rufman reopened this May 8, 2017
@rufman
Copy link
Contributor Author

rufman commented May 9, 2017

I seem to have issues with continuous integration timing out on different tasks on every rebuild attempt. Is there a way to get the build to pass?

@bestander
Copy link
Member

@rufman, thanks for the fix, looks great.
But could you come up with a more focused test?
nic produces a 1000 lines lockfile, test fixtures like that slow down tests runtime and make CI less stable.

Also you need to actually add code to the https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/check.js that would execute the fixture and verify that warning is not printed.

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Test needed

@rufman
Copy link
Contributor Author

rufman commented May 12, 2017

Ok, so I assume I would need to publish some test package that only has one dependency and one bundled dependency?

@bestander
Copy link
Member

bestander commented May 12, 2017 via email

@rufman rufman force-pushed the master branch 3 times, most recently from 598f1d3 to a88901f Compare May 15, 2017 23:45
Any package with bundledDependencies will be added to a hash of bundled
deps, which are used to check against when printing dedup warnings.
This is because bundled dependencies could result in duplications which
we would otherwise detect as false positives.

Fixes yarnpkg#3299
@rufman
Copy link
Contributor Author

rufman commented May 18, 2017

@bestander I've rebased and added a test. The fixture now only contains one bundled dependency, so it should be much lighter.

@bestander bestander merged commit dac451d into yarnpkg:master May 19, 2017
@bcoe
Copy link

bcoe commented May 21, 2017

@rufman awesome work, thanks 👍

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.

3 participants