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

feat: attempt to resolve exact npm package version from lockfiles for constraint checking #170

Merged
merged 2 commits into from
Aug 15, 2022
Merged

feat: attempt to resolve exact npm package version from lockfiles for constraint checking #170

merged 2 commits into from
Aug 15, 2022

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jul 8, 2022

This resolves the core of #156 - I think there's some followup improvements that could be made, but this is the first big step towards that. That's also why I've gone with this implementation which sticks with the current behaviour except now it tries to grab the more exact version from a lockfile if possible; imo it would be good for this to also handle if a package.json or lockfile doesn't exist since I don't think shakapacker the Gem is expected to be used with shakapacker the npm package (though I might be wrong) and not having a lockfile is valid solution to using relaxed constraints.

Originally I posted a full blown parser for yarn.lock files but we don't actually need to do all that work when we're just interested in the version of shakapacker specified in the lock (of which there should only be one, and it should sit at the top level - that's another situation that could be improved in a follow-up PR, to have shakapacker tell people that's a problem if it occurs); so I've gone with a smaller dedicated parser that focused on just finding the first instance of the package we want (which in this case is shakapacker).

I've not really used minitest before, so I've just duplicated the tests four more times (one for each lockfile type & version), but it'd be nice if there's way to say have one base class full of tests that is the parent of the four lockfile tests.

Resolves #156

Copy link
Member

@justin808 justin808 left a comment

Choose a reason for hiding this comment

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

LGTM.

@justin808 justin808 requested a review from tomdracz August 2, 2022 09:16
@justin808 justin808 merged commit 291e8e3 into shakacode:master Aug 15, 2022
@G-Rath G-Rath deleted the improve-node-constraint-handling branch August 19, 2022 20:29
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.

Consider relaxing the version check and making it more consistent
2 participants