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(arborist): tag spec should install the exact version #5599

Closed
wants to merge 2 commits into from

Conversation

gemwuu
Copy link

@gemwuu gemwuu commented Sep 28, 2022

Given package.json below:

{
  "dependencies": {
    "kewu": "1.0.0",
    "wuke": "1.0.1"
  }
}

The two npm package dependencies are:

  1. kewu@1.0.0 has only one dependency wuke@stable
  2. wuke@stable refers to wuke@1.0.0

Run code below, would only install wuke@1.0.1 into the root dir of node_modules.

const arborist = new Arborist({
  path: process.cwd(),
})
const idealTree = await arborist.buildIdealTree({})

console.log('idealTree wuke: %s', idealTree.children.get('kewu').children.get('wuke'))

@npmcli/arborist treats wuke@1.0.1 as the matched version of wuke@stable, which is wrong.

The PR intends to fix this.

As tested, this bug exists from the very beginning implement of arborist.

References

@gemwuu gemwuu requested a review from a team as a code owner September 28, 2022 13:47
@gemwuu gemwuu changed the title fix: tag spec should install the exact version fix(arborist): tag spec should install the exact version Sep 28, 2022
@wraithgar
Copy link
Member

I think there is a very specific reason this works the way it does. I am pretty sure this will break every npm ci out there if implemented, and the deps they have referenced by tag publish new versions under that tag. The tag resolution with a lockfile works the same way as git resolution. A tag is not idempotent, so once it's in the tree we can't re-evaluate it without making older installations invalid.

The way npm works now is that if you give it a tag, it resolves that tag and then "remembers" that resolution via lockfile. Whether the tag came from the package.json or part of add is irrelevant.

This does outline another issue and that is package.json files really should not have tags in them, for this very reason. A tag does not obey semver, and can point to anything. It can easily move out from under you and should be considered an antipattern.

This change would be a breaking change, and I don't think it's on the roadmap for 9.0. We're trying to make a 9.0 that works for most people who are already using 8, and this would definitely not fall under that criteria.

@gemwuu
Copy link
Author

gemwuu commented Sep 30, 2022

@wraithgar So, this behavior change will file for now, and integrate on the next major version(npm 10.0)? Should I create an RRFC for further discussion?

@wraithgar
Copy link
Member

Yes at the very least this needs further discussion.

@wraithgar wraithgar closed this Oct 3, 2022
@gemwuu
Copy link
Author

gemwuu commented Oct 4, 2022

Yes at the very least this needs further discussion.

RRFC: npm/rfcs#640

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

2 participants