Reaction: bin/package-unlink unlinks all packages instead of the one that was specified

[loan-laux]

Prerequisites

• [x] Are you running the latest version?
• [x] Are you able to consistently reproduce the issue?
• [x] Did you search the issue queue for existing issue? Search issues

Issue Description

Running bin/package-unlink some-plugin (works for all plugins) seems to unlink all plugins and not just some-plugin. This is the case regardless of whether the plugin was linked with bin/package-link beforehand or not.

Steps to Reproduce

1. Link a plugin with bin/package-link some-plugin, then link another plugin with bin/package-link other-plugin.
2. Open a shell on your API container with docker exec -it reaction_api_1 bash.
3. In your container's shell, go to /usr/local/src/app/node_modules/@reactioncommerce.
4. Still in your container's shell, run ls -la. Notice you have two symlinks in place: one for some-plugin and another for other-plugin.
5. On your host machine, run bin/package-unlink some-plugin to unlink some-plugin.
6. Back into your container's shell, run ls-ls. Notice that both some-plugin and other-plugin were unlinked.

Possible Solution

No idea. Is this even on Reaction's side or is it an NPM issue?

[aldeed]

This is yucky. This is indeed an npm unlink issue, and there's an accepted RFC to make it work correctly here: https://github.com/npm/rfcs/blob/latest/accepted/0011-npm-link-changes.md

But it seems that won't be until at least npm@7.

I've experimented a bunch and there appear to be a few different bugs. Some I can fix. I have a draft PR here: https://github.com/reactioncommerce/reaction/pull/6251

I'd like to also figure out a way to maintain the other links when unlinking one, but I think we'd have to create a file to track the links and re-create them all in the unlink script after unlinking. In other words, essentially write our own version of the NPM RFC proposal.

A bigger concern is that I am seeing that often, after I link a second package, the code from the first package is no longer executing, even though the symlink is still there and the symlinked files have the changes. It's like there's a different version of the package also installed somewhere in the node_modules tree, and the import() is finding that one instead. So far I haven't figured out what's going on.

[loan-laux]

@aldeed The implementation of this RFC can't come soon enough! Things will be a lot better when this will be available for us to use.

As for your last paragraph, do your symptoms match by any chance what I've noted at #6250?

[aldeed]

@loan-laux Yes! I hadn't thought about the cache. I don't know why that would override what's actually there but it's an idea to explore.

[mikemurray]

I'm also seeing issues where package-unlink things a package isn't symlinked, even though it most definitely is.

npm link and docker are very problematic together that I'd rather see us use something else like yalc (https://github.com/whitecolor/yalc). I've tested with the new admin at the start and it was far less buggy.

[loan-laux]

@mikemurray Since @aldeed's latest changes, I've found that I needed to use bin/package-unlink @reactioncommerce/package-name instead of just bin/package-unlink package-name. Did this occur in both cases?

However, I do agree that Yalc looks like a much better solution. This looks like what we should implement!

[mikemurray]

@loan-laux your suggestion worked for me. I think there was a disconnect for me between the plugin name used during the link is different than the unlink.

I think a quick fix to the current script could be, either an error message suggesting Did you mean @reactioncommerce/plugin_name or trying @reactioncommerce/$plugin_name first if there's no prefix, then fallback to previous logic.

[loan-laux]

@mikemurray 100% agreed.

[aldeed]

Definitely should be using the full pkg name for both link and unlink now. It's done that way so that non-Reaction pkgs can be linked, too. But I agree there could be ways to add sanity checks.

I think it would also be easy to add a flag or something telling it to use yalc instead of link. Most of the script other than the link command would be the same (if I'm understanding yalc). Only problem is I think you'd need to update the node-dev Docker images to have yalc preinstalled.

[mikemurray]

This past week, I've been running the yalc commands locally and have it do it's thing. Everything seems to work fine, but this does mean that you have to globally install yalc. Since we run package-link and package-unlink out side of docker it should be fine, but I can't be sure until we try.

The steps I've used for linking packages and running tests.

# Install yalc globally
npm i -g yalc

# Go you the plugin, or package you want to publish
cd api-plugin-<name>/

# Publish package
yalc publish

# Go to reaction
cd reaction/

# Publish (use one of the following, preferably link)
yalc link @reactioncommerce/api-plugin-<name> # Safer, doesn't add to package.json
yalc add @reactioncommerce/api-plugin-<name> # Adds to package.json so be careful

# Make changes in the package and republish
cd api-plugin-<name>/
yalc push # Publishes and auto updates reaction (or any project that contains this as yalc dependency)