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

Bugfix 3306: Check top level patterns during integrity check #3811

Merged
merged 1 commit into from Jul 5, 2017
Merged

Bugfix 3306: Check top level patterns during integrity check #3811

merged 1 commit into from Jul 5, 2017

Conversation

ghost
Copy link

@ghost ghost commented Jul 3, 2017

Summary
Fixes issue 3306. Previously changes to the package.json dependencies would not throw an error during integrity checks, unless there were zero dependencies, in which case an empty yarn.lock would be written.

A condition was added to the integrity checker for top level patterns. If the top level patterns are not the same, the install task will continue running with the force flag set, as would be done with a reinstall after a package was removed.

yarn check --integrity is also affected by this, and will return an error if the top level patterns don't match those specified in the package.json.

Test plan
Before:

yarn add foo left-pad
# package.json is edited to remove left-pad
yarn
# left-pad is still in yarn.lock
# package.json is edited to remove foo
yarn
# yarn.lock is empty

After:

yarn add foo left-pad
# package.json is edited to remove left-pad
yarn
# yarn.lock only contains foo
# package.json is edited to remove foo
yarn
# yarn.lock is empty

if (!match.integrityMatches && match.integrityError == 'PATTERNS_DONT_MATCH') {
this.reporter.warn(this.reporter.lang('integrityPatternsDontMatch'));
this.reporter.info(this.reporter.lang('uninstallRegenerate'));
this.flags.force = true;
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we should force here

Copy link
Author

Choose a reason for hiding this comment

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

Okay, should the install command not regenerate the lockfile then, and instead just display a warning in this case?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should cleanup lockfile, yes.
Just force may mean more things like forcing rebuilding binary dependencies.

Copy link
Member

Choose a reason for hiding this comment

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

Probably the logic for missing dependencies should be done not in bailout function but where the lockfile is saved

Copy link
Author

Choose a reason for hiding this comment

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

Got it. I'll try that out instead.

@bestander
Copy link
Member

Thanks, @aracarie.
Would you add a test please?
Here is an example https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/integration.js#L445

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.

Thanks for the fix!
Prettier still complains.

Please rebase, run yarn install && yarn prettier.
After CI is green it is ready to be merged

@ghost
Copy link
Author

ghost commented Jul 5, 2017

👍

done

@bestander bestander merged commit 47179ff into yarnpkg:master Jul 5, 2017
@ghost ghost deleted the check-top-level-patterns branch July 5, 2017 23:17
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.

1 participant