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

fix-679: empty lockfiles with no dependencies #1703

Merged
merged 3 commits into from Dec 15, 2016
Merged

fix-679: empty lockfiles with no dependencies #1703

merged 3 commits into from Dec 15, 2016

Conversation

ghost
Copy link

@ghost ghost commented Nov 5, 2016

Summary

No lockfile is written with empty dependencies, which causes check to fail. Act the same with manifest & lockfile, so write an empty file as-well when no dependencies are going to be installed dependencies: {}.

This will solve: #679

Test plan

$ yarn init
$ yarn install
$ yarn check
$ yarn check --integrity
$ npm run test

@bestander
Copy link
Member

How about a unit test?

@ghost
Copy link
Author

ghost commented Nov 6, 2016

Yes, I was going to, but it was super late already, added it now. I'm sorry, should have made that clear.

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.

Hey @verth, thanks for adding a test case.
I am currently working on a fix that would refactor this inSync section and we won't need writeEmpty flag case.
However it would be great to add your test anyway, could you rebase your PR after the other one is merged?
I'll add a link here in an hour

@bestander
Copy link
Member

#1712

@ghost
Copy link
Author

ghost commented Nov 7, 2016

Cool, cool! Will keep my eye on it

@bestander
Copy link
Member

Ping #1712 is merged

@ghost
Copy link
Author

ghost commented Nov 14, 2016

I tried to kill the commits and merge master with the test only but it was not passing. When testing install manually without dependencies it also did not write the lockfile for me (with v.0.18.0 last commit 15d0112).

Would you know something more by any chance, intentional? (not pushed yet to keep fix alive atm)

@bestander
Copy link
Member

@verth, I don't think we had intentional behavior around edge cases with no dependencies.
I think integrity check should pass for empty lockfile, so go ahead rebase and give it another try if you want

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.

rebase needed

@ghost
Copy link
Author

ghost commented Dec 13, 2016

@bestander, rebased and dropped the "code fix" in PR to rely on isSync fix, seems like something small might still be a little off? Since rebase test it not passing, tested it manually as-well and it seemed the yarn.lock was not created.

@bestander
Copy link
Member

@verth, currently yarn does not write lockfile if there are no dependencies.
A change in install.js is needed

@ghost
Copy link
Author

ghost commented Dec 14, 2016

@bestander Reimplemented a fix now as-well.

My bad this was dragged out so long, thought you only wanted to keep the test in this PR, I probably misinterpreted "currently working on a fix [...] However it would be great to add your test anyway".

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.

None yet

1 participant