Improve third-party Astro package support

[delucis]

Changes

Fix #4071

Search for Astro package dependencies beyond a project’s direct dependencies.

See this comment for more background — it lays out some alternative approaches, I’d be curious what people think!

TL;DR Current logic only adds direct dependencies to vite.ssr.noExternal. The changes in this PR walk the dependency tree, checking child dependencies of any Astro packages it finds recursively. It bails out of a branch when it encounters a non-Astro package. This should still be reasonably performant, while being much more robust.

Testing

Help would be appreciated. @natemoo-re’s original PR for this code (#2665) mentions:

Not sure there's a great way to test this.

What I did manually:

1. pnpm build in this repo
2. cd packages/astro && npm pack to get a tarball for the new version
3. Create a new minimal Astro project locally
4. Install astro-embed and use its YouTube component:

   ---
   import { YouTube } from "astro-embed";
   ---
   <YouTube id="https://youtu.be/xtTy5nKay_Y" />

5. npm start — encounters errors as reported in 🐛 BUG: astro-embed integration fails due to .astro file extension error #4071
6. npm i ../astro/packages/astro/astro-1.1.5.tgz to install Astro from the tarball
7. npm start — errors go away! ✨

Docs

Bug fix for an issue that we’re not documenting in detail yet, although this is related to withastro/docs#1455.

[changeset-bot]

🦋 Changeset detected

Latest commit: a7dfbd7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages

Name	Type
astro	Patch
@e2e/astro-component	Patch
@e2e/error-react-spectrum	Patch
@e2e/error-sass	Patch
@e2e/errors	Patch
@e2e/hydration-race	Patch
@e2e/lit-component	Patch
@e2e/preact-component	Patch
@e2e/react-component	Patch
@e2e/solid-component	Patch
@e2e/solid-recurse	Patch
@e2e/svelte-component	Patch
@e2e/e2e-tailwindcss	Patch
@e2e/ts-resolution	Patch
@e2e/third-party-astro	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[bluwy]

This make sense to me. We have the same issue for vite-plugin-svelte (.svelte files and .astro files are exported raw), and this recursive technique is what we came up too. Reference: dependencies.ts and usage.

Admittedly this is a problem in Vite that we hadn't found a nice solution yet. Ideally nodejs ESM loaders should help when that's stable.

In my second linked code, you'll also notice that we externalize transitive deps too, to prevent transitive CJS deps reporting a Vite caveat like "module" is not defined error in dev mode. We could probably improve that in the future too.

[delucis]

Thanks @bluwy! I had a suspicion you’d have some good insights into this issue 💜

[matthewp]

Can a test be added?

[delucis]

Can a test be added?

Nate scared me off with his comment, but I can try!

[delucis]

Test added! I’ve also had to switch up my fix to get the tests passing, so probably a good call to try testing this.

Our existing package.json look-up strategy simply loaded node_modules/${dep}/package.json and hoped that was the correct location. That fails in monorepo environments where node_modules might be elsewhere making this impossible to test (and presumably also brittle for monorepo Astro users).

I’ve switched to using createRequire to resolve dependencies based on the vite-plugin-svelte strategy @bluwy linked to.

My test uses astro-embed which is the community package (which I maintain) that triggered the initial bug report. This is in the @astro-community org and under the @astro-community scope on npm, the latter of which I am the admin for. Not sure if there’s a better way to test this — I guess it would be hard to create a third-party package with indirect astro dependencies within the astro monorepo?

[bluwy]

The new changes looks great!

Not sure if there’s a better way to test this — I guess it would be hard to create a third-party package with indirect astro dependencies within the astro monorepo?

I think the current test method is fine, but if we want to avoid an external dependency, we can use the file: protocol from pnpm so that it hardlinks the package instead of symlink, which can emulate installing a third-party package.

So you'd have something like: "astro-lib": "file:../path/to/lib" instead of "astro-lib": "workspace:*". Vite also does this too.

[matthewp]

We do have some tests that use local packages, like this one: https://github.com/withastro/astro/blob/main/packages/astro/test/fixtures/css-assets/package.json

But I think this method is fine for now.

[delucis]

Thanks @bluwy & @matthewp! I think ultimately it would be better to have an internal package to test this, but I’ve already sunk quite a bit of time into this one, so I’ll merge and happy for us to tackle that down the road!

[beeb]

This PR makes the linter fail now:

astro/packages/astro/src/core/create-vite.ts
  246:13  error  'e' is already declared in the upper scope on line 232 column 12  @typescript-eslint/no-shadow

when running pnpm run lint. See https://github.com/withastro/astro/runs/8226710446
Not sure why CI didn't catch it..