Fix #570 missing package not resolved

[JennieJi]

Changes

To support babel macros.

In dev mode, before reinstalling dependencies, do add the missing entry points to knownEntrypoints as reinstall will only install the dependencies from configuration, and imports scanned from the source code.

Before:

After:

Testing

Tested here: https://github.com/JennieJi/snowpack/pull/3

I only reproduced this via babel macros, but installing babel plugin will add too many files. And I didn't have a good idea of how to use the current test runners to test the dev mode... Thus I chose not to add in this branch. @FredKSchott need some advice about this.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/6qwk7t830
✅ Preview: https://snowpack-git-fork-jennieji-jennie-add-missing-entrypoint.pikapkg.vercel.app

[JennieJi]

Another thinking here is whether it's possible to run the reinstallDependencies() after transformFileImports() instead of running in each callback functions

[FredKSchott]

This is a good start, but adding it to the knownEntrypoints config property won't save this back to your config, which means that next time you run any Snowpack command this new dependency name will be gone again. I think instead, we need to warn the user with a nice message so that they can add it to their own knownEntrypoints config and have it persist across runs.

[FredKSchott]

commented!

[JennieJi]

This is a good start, but adding it to the knownEntrypoints config property won't save this back to your config, which means that next time you run any Snowpack command this new dependency name will be gone again. I think instead, we need to warn the user with a nice message so that they can add it to their own knownEntrypoints config and have it persist across runs.

Can, will add a warning in terminal

[JennieJi]

Hmmm the logs will be cleared somehow, may be related to #558 ?

[FredKSchott]

This is definitely closer, but we still want to avoid pushing the package name into config.knownEntrypoints and should treat config as readonly.

One other tricky part of this is that exists could be true if the package was installed, and then this is now the first use of it in your application. In that case, the user doesn't need to add it to knownEntrypoints.

Here's the solution that I think would work for both usecases:

 const doesPackageExist = !!depManifestLoc;
+const doesImportExist = scanImportsFromFiles().some((target => target.specifier === spec);

 missingWebModule = {
   spec: spec,
   pkgName: packageName,
+  packageExists: doesPackageExist,
+  importExists: doesImportExist
 };

Then, the paint function would have all it need:

• if !doesImportExist, then tell the user to add to install: [] in their config.
• if !doesPackageExist, then tell the user to "npm install" the package.
• otherwise, it's safe to run a run an automatic "snowpack install" for them.

[FredKSchott]

looks like missingWebModule is always null here, since you return immediately after setting missingWebModule = { ...

[FredKSchott]

PS: apologies for the noise, but we just converted this project to a monorepo. We tried to preserve history in the repo, so you should be able to rebase/merge to get the latest changes.

[JennieJi]

Thanks, I will try later.

[JennieJi]

Thanks, I will try later.

[JennieJi]

Hi @FredKSchott ,

I've removed the mutation on config object.

Then I find the reason why missingWebModule was always null because of the reinstalling will pass INSTALL_COMPLETE to message bus and there it will reset the missingWebModule. And it works fine after removal of reinstalling:

And for doesImportExist with rescanning the imports, I don't really get why do we need a rescanning to confirm whether the import exists? In my understanding, this part is under the callback function of transformFileImports(), so it has confirmed the import exists I suppose?

[FredKSchott]

Update: We ended up running into separate issues with this "auto install missing packages" workflow and the new dev output tracked in #648, which forced us to just remove it entirely for now.

I went ahead and added this message about config.install to the new warning that we output (a9d2f4b) which should solve your use-case and is hopefully less complex for everyone.

Thanks again for your work here! if you are able to test against master and see anything still not working, please let me know and I'll take a look. Looking forward to getting this released and fixing the macros issue!

[JennieJi]

Noted thanks