Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

link target metadeps not saved in legacy shrinkwrap dependencies properly #84

Closed
isaacs opened this issue May 23, 2020 · 0 comments
Closed

Comments

@isaacs
Copy link
Contributor

isaacs commented May 23, 2020

Given a folder structure like this:

.
├── node_modules
│  └── a -> ../once
├── once
│  ├── node_modules
│  │  └── wrappy
│  │     └── package.json
│  └── package.json
├── package-lock.json
└── package.json

npm v6 produces a shrinkwrap like this:

{
  "requires": true,
  "lockfileVersion": 1,
  "dependencies": {
    "a": {
      "version": "file:once",
      "requires": {
        "wrappy": "1"
      },
      "dependencies": {
        "wrappy": {
          "version": "1.0.2",
          "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
          "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8="
        }
      }
    }
  }
}

But arborist is doing this in the legacy shrinkwrap section:

{
  "dependencies": {
    "a": {
      "version": "file:once"
    },
    "once": {
      "dependencies": {
        "wrappy": {
          "version": "1.0.2",
          "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz",
          "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8="
        }
      }
    }
  }
}

That's wrong.

It seems like the thing to do here is start from the root Node object, and walk the children (using target.children for links) and build the object that way.

The Shrinkwrap class needs to keep a reference to the tree representing its root node (replacing if we ever give it a new node at the '' location), and should have a Map of location -> metadata. If there are nodes that don't end up being actualized, we can still write back the metadata for those locations. (So, for example, a reify() that omits dev deps will still have them in the lockfile). Then we can also trivially write the yarn.lock on save(), solving #82.

The reason why it didn't do this initially was a notion that Shrinkwrap and Node should be separate concerns, and shouldn't know too much about each other. However, in practice, they're all up in each other business anyway, and we're doing a lot of translating path strings into object paths and so on. Since the two classes really can't live apart from one another, we may as well have a more straightforward way of having them interact.

isaacs added a commit that referenced this issue May 27, 2020
Since the Shrinkwrap class was developed prior to figuring out exactly
how it would be used in the various Arborist methods, some things ended
up growing in a somewhat inelegant direction.

- Nodes were fetching their resolved and integrity values in multiple
  places.
- The Shrinkwrap internal data tried to keep up both the new and old
  data up to date as the tree changed, but this was not always possible.
- Dependencies of link targets were not properly reflected in the legacy
  shrinkwrap metadata.
- The Node class and buildIdealTree methods both had to dive deep into
  the internals of the YarnLock class, which should be an implementation
  detail of the Shrinkwrap class.

Now, the Shrinkwrap object keeps a reference to the root tree node found at
its own path.  Rather than try to keep the legacy metadata in sync at
all times, we _only_ build up the legacy shrinkwrap data when calling
`this.commit()`, by walking the `node.children` and
`node.target.children` maps appropriately.

Also, `this.yarnLock` is updated on commit(), and a `checkYarnLock()`
method is added to return the spec that should be fetched, and update
the provided options object with resolved and integrity metadata.

All of the logic for setting node metadata from the Shrinkwrap is now
done in `Shrinkwrap.add(node)`, making it much simpler and harder to get
wrong.

This means that:

- `yarn.lock` files are respected more fully (including their resolved
  and integrity expectations)
- We don't have cases where both `extraneous` and `dev`/`optional` are
  set in the lockfile (which is always an error -- extraneous nodes
  cannot be either dev or optional!)
- Link target dependencies are accurately reflected in the legacy
  shrinkwrap data, rather than creating invalid `../` entries.

Fix: #82
Fix: #84
isaacs added a commit that referenced this issue May 27, 2020
Since the Shrinkwrap class was developed prior to figuring out exactly
how it would be used in the various Arborist methods, some things ended
up growing in a somewhat inelegant direction.

- Nodes were fetching their resolved and integrity values in multiple
  places.
- The Shrinkwrap internal data tried to keep up both the new and old
  data up to date as the tree changed, but this was not always possible.
- Dependencies of link targets were not properly reflected in the legacy
  shrinkwrap metadata.
- The Node class and buildIdealTree methods both had to dive deep into
  the internals of the YarnLock class, which should be an implementation
  detail of the Shrinkwrap class.

Now, the Shrinkwrap object keeps a reference to the root tree node found at
its own path.  Rather than try to keep the legacy metadata in sync at
all times, we _only_ build up the legacy shrinkwrap data when calling
`this.commit()`, by walking the `node.children` and
`node.target.children` maps appropriately.

Also, `this.yarnLock` is updated on commit(), and a `checkYarnLock()`
method is added to return the spec that should be fetched, and update
the provided options object with resolved and integrity metadata.

All of the logic for setting node metadata from the Shrinkwrap is now
done in `Shrinkwrap.add(node)`, making it much simpler and harder to get
wrong.

This means that:

- `yarn.lock` files are respected more fully (including their resolved
  and integrity expectations)
- We don't have cases where both `extraneous` and `dev`/`optional` are
  set in the lockfile (which is always an error -- extraneous nodes
  cannot be either dev or optional!)
- Link target dependencies are accurately reflected in the legacy
  shrinkwrap data, rather than creating invalid `../` entries.

Fix: #82
Fix: #84
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant