🐛 BUG: Astro packages don't properly define their dependencies (Yarn PnP incompatibilty)

andreialecu commented 3 years ago

What package manager are you using?

yarn 3.1

What operating system are you using?

Mac

Describe the Bug

I've been experimenting with Astro and Yarn 3 in PnP mode.

PnP mode is strict about packages requiring dependencies, and it does not allow them to be hijacked from other packages.

Astro is doing some things wrong in this regard, for example it uses fast-glob but it's nowhere in package.json. I suspect the pnpm package manager will also have problems with this.

Steps to Reproduce

yarn create astro --template framework-react
yarn set version berry
yarn
yarn dev

➜ yarn dev       
(node:91510) ExperimentalWarning: --experimental-loader is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
Error: astro tried to access fast-glob, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

Required package: fast-glob (via "fast-glob/package.json")
Required by: astro@npm:0.20.12 (via /.../.yarn/cache/astro-npm-0.20.12-5a08c46146-1be4d53e64.zip/node_modules/astro/dist/check.js)

fast-glob is indeed imported here: https://github.com/snowpackjs/astro/blob/b1b564d03d09cee63e072397dfb2ab1a1f59b346/packages/astro/src/check.ts#L5

But it's not defined in any package.json.

This is a correctness issue. See: https://yarnpkg.com/advanced/rulebook/#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

Link to Minimal Reproducible Example (Optional)

No response

andreialecu commented 3 years ago

I looked into this a bit further and was able to get astro to start after adding a lot of missing dependencies.

In particular, these packageExtensions were needed:

packageExtensions:
  "astro@*":
    dependencies:
      fast-glob: "*"
  "@astrojs/language-server@*":
    dependencies:
      vscode-uri: "*"
      vscode-languageserver-types: "*"
  "@babel/plugin-transform-react-jsx@*":
    dependencies:
      "@babel/core": "*"

Also needed to update the package.json to add a bunch of extra dependencies that were missing:

  "dependencies": {
    "@astrojs/renderer-react": "^0.2.2",
    "@snowpack/plugin-postcss": "^1.4.3",
    "@snowpack/plugin-sass": "^1.4.0",
    "astring": "^1.7.5",
    "autoprefixer": "^10.3.7",
    "estree-util-value-to-estree": "^1.3.0",
    "node-fetch": "^3.0.0",
    "object-assign": "^4.1.1",
    "postcss": "^8.3.11",
    "prismjs": "^1.25.0",
    "react": "^17.0.2",
    "react-dom": "^17.0.2",
    "shorthash": "^0.0.2"
  }

Now there's this error:

Failed to load "node-fetch"!
ESM format is not natively supported in "node@v16.9.0".
Please use CommonJS or upgrade to an LTS version of node above "node@12.17.0".
(node:2199) ExperimentalWarning: stream/web is an experimental feature. This feature could change at any time
[executing astro] TypeError: __import1.generate is not a function
    at serialize (/_snowpack/pkg/astro.dist.internal.__astro_component.v0.20.12.js:48:40)
    at generateHydrateScript (/_snowpack/pkg/astro.dist.internal.__astro_component.v0.20.12.js:119:53)
    at __astro_component_internal (/_snowpack/pkg/astro.dist.internal.__astro_component.v0.20.12.js:211:26)
    at async Promise.all (index 0)
    at async Object.h (/_snowpack/pkg/astro.dist.internal.h.v0.20.12.js:45:20)
    at async Promise.all (index 0)
    at async Object.h (/_snowpack/pkg/astro.dist.internal.h.v0.20.12.js:45:20)
    at async Promise.all (index 1)
    at async Object.h (/_snowpack/pkg/astro.dist.internal.h.v0.20.12.js:45:20)
    at async Promise.all (index 1)

andreialecu commented 3 years ago

I created a repo at https://github.com/andreialecu/astro-yarn3-pnp with my initial research into this.

It includes all missing dependencies (I added them manually), and shows the ESM error.

natemoo-re commented 3 years ago

Thanks for digging into this! Many of these issues should be fixed in #1406. As for Yarn 3 PnP, https://github.com/yarnpkg/berry/pull/2161 suggests that this particular ESM issue will be fixed in an upcoming version, if it has not already landed.

drwpow commented 3 years ago

Minor update on this: I’ve been testing astro@next (0.21 beta) locally using the latest version of pnpm with no issues. I believe pnpm support should be fixed with the upcoming release. Unknown about Yarn 3.

FredKSchott commented 2 years ago

Confirmed issues in astro@next on yarn berry mode. I think pnpm still has the same issues, mainly around our renderer loading system. We can aim to prioritize this post v0.21.0 release

tony-sull commented 2 years ago

Just confirmed I'm still seeing this with yarn berry

Looks like https://github.com/yarnpkg/berry/pull/2161 is merged in now. I'm not super familiar with the inner workings of the different package managers, is this something we should fix on our end or is it blocked on a berry issue?

FredKSchott commented 2 years ago

Astro monorepo is now on pnpm, so this should be mostly resolved. However, shamefully-hoist is still recommended.

MrAlex94 commented 2 years ago

@FredKSchott, I think this has been incorrectly closed. pnpm is separate from yarn's Plug'n'Play (pnp), which this thread is talking about.

I have this issue, and need to add extra dependencies with yarn. Even with the dependencies fixed, astro still does not work with pnp.

bluwy commented 2 years ago

@MrAlex94 we recently fully support projects without shamefully-hoist, which is a common issue with strict dependency installations like pnpm and yarn pnp. Are there anything else specific that we didn't cover? I might be better to create a separate issue for.

withastro / astro