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: update notifier to consider engine range #8050

Draft
wants to merge 1 commit into
base: latest
Choose a base branch
from

Conversation

milaninfy
Copy link
Contributor

@milaninfy milaninfy commented Jan 21, 2025

When doing manifest call for fetching npm manifest it was using latest every-time, however fetchingnpm@* will still give latest if it's satisfies the engine range otherwise it will give highest matching version for the current engine range.
Pacote.manifest calls internally uses pick-manifest logic to pick the appropriate version of the manifest for the engine range.

@milaninfy milaninfy marked this pull request as ready for review January 22, 2025 04:27
@milaninfy milaninfy requested a review from a team as a code owner January 22, 2025 04:27
@wraithgar
Copy link
Member

We'll want a new test for the behavior this is guarding against.

@milaninfy milaninfy marked this pull request as draft January 22, 2025 19:22
@milaninfy
Copy link
Contributor Author

while thinking on how to write a test for this one, logically the problem this pr solves it to get correct engine matched version, we only request for manifest from pacote with appropriate spec. earlier we were requesting npm@latest which gave us latest no matter what the node version is, but now we are fetching manifest with npm@* which gives us default tag ( latest ) if nothing matched otherwise highest appropriate version with engine consideration ( This logic is from npm-pick-manifest )
Do we need an end to end test here to show that it's marching based on engine ?

@wraithgar
Copy link
Member

Do we need an end to end test here to show that it's matching based on engine ?

Yes, this is behavior we don't want to accidentally regress from. The update notifier tests will likely need a bit of tweaking to get there

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.

2 participants