Reaction: Linking a local api-core throws error about missing api-plugin-simple-schema

[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

If you link in a local api-core with bin/package-link api-core, your API will restart and throw the following error:

ERROR Reaction: Cannot find package '@reactioncommerce/api-plugin-simple-schema' imported from /home/node/copied-packages/api-core/src/importPluginsJSONFile.js

Steps to Reproduce

1. Link in your local api-core package with bin/package-link api-core.
2. Your API restarts. Notice that it throws the above-mentioned error.

Possible Solution

Since api-core is the package that imports all other plugins from the plugins.json file, I think it's somehow looking for them (or maybe their own dependencies) in /home/node/copied-packages/. Just a guess though. Could be something else.

[loan-laux]

I've been digging a bit more into this issue and have been able to work around it. Not in a very clean way though, so I still think it should be fixed so as to necessitate no intervention from developers whatsoever.

First, Reaction is complaining about @reactioncommerce/api-plugin-simple-schema being missing simply because it's the first package listed in the plugins.json file.

It can't import any of them because api-core is now in /home/node/copied-packages/ and not amongst the rest of the project's code, so NPM is looking for these dependencies which obviously aren't here.

I was able to "fix" it by replacing the reference to the NPM package names in plugins.json by a relative path pointing to the actual location of the NPM package in the container. This is what it looks like in practice:

{
  "simpleSchema": "../../../../usr/local/src/app/node_modules/@reactioncommerce/api-plugin-simple-schema/index.js",
  "jobQueue": "../../../../usr/local/src/app/node_modules/@reactioncommerce/api-plugin-job-queue/index.js",
  "files": "../../../../usr/local/src/app/node_modules/@reactioncommerce/api-plugin-files/index.js",
  // etc...
}

Now, it looks like we have two ways to fix this:

1. In api-core's importPluginsJSONFile.js, check if the current working dir is /home/node/copied-packages/api-core (i.e. if this is a locally linked version), and automatically generate the correct paths from the plugins' names before importing them.
2. Somehow npm link the packages from /home/node/copied-packages/api-core? Don't know if that's possible nor how that would work in practice.

[aldeed]

@loan-laux Thanks for investigating. Here's basically what I think is happening here (mostly summarizing what you already said).

The importPluginsJSONFile function has fancy handling for any plugin that is a file path, to ensure it can still be resolved when doing dynamic import() from api-core. But when it's referencing a package name, we don't do anything special. We just pass the package name to import() first argument.

https://github.com/reactioncommerce/api-core/blob/trunk/src/importPluginsJSONFile.js#L44

This means that it will resolve that from the copied and linked api-core, which is no longer in the main project and has its own node_modules.

Some form of your first proposed solution seems possible. I haven't tried it yet, but a solution might be something like this:

const resolvedPackageRelativeToJSONFile = require.resolve(pluginPath, { paths: [path.dirname(absolutePluginsFile)] });
({ default: plugin } = await import(resolvedPackageRelativeToJSONFile));

[loan-laux]

Thanks for the feedback @aldeed. I'll PR something along those lines very soon!

[loan-laux]

@aldeed I just took some time to work on this. It seems require.resolve can't be used inside of modules. The recommended alternative is import.meta.resolve, which was introduced with Node 14 and uses the --experimental-import-meta-resolve flag.

An update to the latest version of Node would be a good thing regardless of this issue, so what do you think of updating to version 14 first?

[aldeed]

@loan-laux I think the goal was to stay on the LTS track of Node. That said, they have been unflagging things on that track. 12.17.0 just removed the need for --experimental-modules flag. Maybe they will backport import.meta.resolve to it soon? v14 goes to LTS in October but it would be nice to solve this sooner if there's a way.

[loan-laux]

@aldeed Gotcha. I'll be on the lookout for a backport of import.meta.resolve to the LTS track.

[delagroove]

@loan-laux is this a good solution?
https://www.npmjs.com/package/import-meta-resolve
a polyfill for the function while we wait for it to be in the LTS version.
just checked and this function has not been added in 14.15.4 which is the latest LTS version.

[loan-laux]

@delagroove Since Node 14.15.4 is now the current LTS, we could use the --experimental-import-meta-resolve flag to make the native implementation work without using this polyfill. @aldeed, thoughts on this?

[aldeed]

I am not opposed to adding the flag in the Docker scripts or anywhere necessary, but I can't speak to whether this is something the core team wants to support. The Node team does sometimes unflag things in a minor release, so it's still possible they will decide to unflag this.

[loan-laux]

Got it, thanks @aldeed. Any thoughts from the core team, @spencern, @mpaktiti etc?