yarnpkg / berry

📦🐈 Active development trunk for Yarn ⚒
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.43k stars 1.11k forks source link

[Bug] yarn remove does not remove entry from `dependenciesMeta` #1490

Open andreialecu opened 4 years ago

andreialecu commented 4 years ago

Describe the bug

When a package is unplugged, running yarn remove does not remove its entry from dependenciesMeta

To Reproduce

Reproduction ```js repro await yarn(`init`, `-y`); await yarn(`add`, `left-pad`); await yarn(`unplug`, `left-pad`); await yarn(`remove`, `left-pad`); const pkgjson = require("./package.json"); expect(pkgjson).not.toHaveProperty("dependenciesMeta") ```
yarnbot commented 4 years ago

This issue reproduces on master:

Error: expect(received).not.toHaveProperty(path)

Expected path: not "dependenciesMeta"

Received value: {"left-pad": {"unplugged": true}}
    at module.exports (evalmachine.<anonymous>:8:21)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async /github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:13
    at async executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:16)
    at async Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:12)
    at async ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.38-d4f5e2dbf3-63f998598d.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:38)
    at async ExecCommand.validateAndExecute (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Command.js:161:26)
    at async Cli.run (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Cli.js:74:24)
    at async Cli.runExit (/github/workspace/.yarn/cache/clipanion-npm-2.0.0-rc.16-b9444aaf89-a57989414f.zip/node_modules/clipanion/lib/advanced/Cli.js:83:28)
brokenmass commented 4 years ago

I don't see why this should be an issue.

Yarn has no way of knowing if you wanted to unplug the "left-pad" that was a direct dependency in your package.json or the "left-pad" that was a dependency of a dependency (or both).

After yarn remove left-pad even if the first one is gone the second might still be there and might still be required to be unplugged. because of that Yarn should not remove the entry from dependenciesMeta

andreialecu commented 4 years ago

It is an issue because removing and readding a dependency should be consistent and it should result in a clean slate.

Users are probably not aware of yarn internals and seeing a previously unplugged package remaining unplugged is probably going to be confusing.

The solution for your concern is to clean it up from package.json if yarn why left-pad returns nothing.

brokenmass commented 4 years ago

yarn remove left-pad is the 'opposite' of yarn add left-pad but is not the opposite of yarn unplug left-pad. The fact that you run the unplug command in between add and remove of that specific dependency is (and should be) irrelevant. unplug does not only apply to direct dependencies and because of that it should not be affected by direct dependencies altering commands (like add or remove)

Your proposed solution to my concern will make the behaviour erratical and a user without the knowledge of yarn internals will have no idea on why removing a dependency sometimes remove the unplug definition and other times it does not. It also will generate an unexpected behaviour is the flow is:

yarn unplug left-pad
yarn remove left-pad
yarn add left-pad //ops i want left-pad back
// at this point the application might stop working as left-pad is not unplugged anymore
andreialecu commented 4 years ago

I respectfully disagree.

What seems erratic to me is that trying to undo the effect of yarn unplug by explicitly removing then adding a dependency back results in it sticking as unplugged without any explanation. Not everyone may be aware of yarn internals, especially with multiple people working on the same project.

A CLI command to undo yarn unplug is probably a good idea to add.

Speaking of consistency, I think leaving trash behind in package.json is hardly consistent as well.