vercel / next.js

The React Framework
https://nextjs.org
MIT License
124.85k stars 26.65k forks source link

Turbopack can't locate serverExternalPackages if installed via pnpm and are child dependencies of other installed modules #68805

Open jmikrut opened 1 month ago

jmikrut commented 1 month ago

Link to the code that reproduces this issue

https://github.com/jmikrut/nextjs-68805

To Reproduce

  1. Install dependencies via pnpm
  2. Run pnpm dev --turbo
  3. Go to http://localhost:3000/admin

Current vs. Expected behavior

Turbopack fails to compile, and the terminal blows up with errors such as the following:

Package @libsql/client (serverExternalPackages or default list) can't be external
The request @libsql/client matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.

If you install dependencies with Yarn, or with npm i --legacy-peer-deps, the dependencies are located correctly with Turbopack.

Note that all dependencies which error are indeed installed as dependencies of installed packages. Webpack works, but Turbopack does not.

Provide environment information

Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 24.0.0: Wed Jul 31 21:51:10 PDT 2024; root:xnu-11215.0.199.501.2~1/RELEASE_ARM64_T6000
  Available memory (MB): 32768
  Available CPU cores: 10
Binaries:
  Node: 20.11.1
  npm: 10.2.4
  Yarn: 1.16.0
  pnpm: 8.15.7
Relevant Packages:
  next: 15.0.0-canary.111 // Latest available version is detected (15.0.0-canary.111).
  eslint-config-next: 15.0.0-canary.104
  react: 19.0.0-rc-06d0b89e-20240801
  react-dom: 19.0.0-rc-06d0b89e-20240801
  typescript: 5.5.4
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Turbopack

Which stage(s) are affected? (Select all that apply)

next dev (local)

Additional context

This appears to have broken in next@15.0.0-canary.84. Worked before that.

samcx commented 1 month ago

@jmikrut Not running into this error myself :thinkies:

68805-turbopack-serverExternalPackages [main] p devturbo

> next-latest-starter@0.1.0 devturbo /.../68805-turbopack-serverExternalPackages
> pnpm dev --turbo

> next-latest-starter@0.1.0 dev /.../68805-turbopack-serverExternalPackages
> cross-env NODE_OPTIONS="${NODE_OPTIONS} --no-deprecation" next dev "--turbo"

  ▲ Next.js 15.0.0-canary.104 (turbo)
  - Local:        http://localhost:3000
  - Experiments (use with caution):
    · turbo
    · reactCompiler

 ✓ Starting...
 ✓ Ready in 1722ms
 ○ Compiling / ...
 ✓ Compiled / in 885ms
 GET / 200 in 965ms
 GET / 200 in 30ms
Operating System:
  Platform: darwin
  Arch: arm64
  Version: Darwin Kernel Version 23.6.0: Mon Jul 29 21:14:46 PDT 2024; root:xnu-10063.141.2~1/RELEASE_ARM64_T6031
  Available memory (MB): 49152
  Available CPU cores: 16
Binaries:
  Node: 18.20.3
  npm: 10.7.0
  Yarn: 1.22.21
  pnpm: 9.7.0
Relevant Packages:
  next: 15.0.0-canary.104 // There is a newer canary version (15.0.0-canary.111) available, please upgrade!
  eslint-config-next: 15.0.0-canary.104
  react: 19.0.0-rc-06d0b89e-20240801
  react-dom: 19.0.0-rc-06d0b89e-20240801
  typescript: 5.5.4
Next.js Config:
  output: N/A
 ⚠ There is a newer canary version (15.0.0-canary.111) available, please upgrade!
   Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
   Read more - https://nextjs.org/docs/messages/opening-an-issue
jmikrut commented 1 month ago

Hey @samcx you need to load up the admin UI (the spot that uses the dependencies in question). I forgot to add that to the reproduction instructions - good catch.

Updated! If you're still not able to repro, I also noticed that you're on pnpm 9.7.0 and I'm on 8.15.7. Might have to do with it.

khuezy commented 1 month ago

I don't get that error (I'm on pnpm 9.7.0) I have a semi-related issue wrt to --turbo

│  ⨯ ./node_modules/@libsql/client/README.md
│ Unknown module type
│ This module doesn't have an associated type. Use a known file extension, or register a loader for it.
│ 
│ Read more: https://nextjs.org/docs/app/api-reference/next-config-js/turbo#webpack-loaders

The serverExternalPackages includes @libsql/client by default, any ideas on why --turbo causes this issue?

samcx commented 1 month ago

@jmikrut It seems error out because I don't have the correct keys. Where do I should I go create my keys (ideally, a :repro: shouldn't require this step)?

jmikrut commented 1 month ago

Ah apologies for that, let me update with a simpler repro that doesn't require keys. 1 sec

jmikrut commented 1 month ago

@samcx I updated with a proper repro that simplifies and does not require environment variables. Should just need to clone the newly linked repro above and run it with turbo to see it fail.

AlessioGr commented 4 weeks ago

Hey @samcx

I created a very, very minimal reproduction of this issue here: https://github.com/AlessioGr/very-minimal-turbo-bug-repro

It's a blank, fresh Next.js project that includes a small, local package in the mydep folder. You can (don't have to, it's pre-built in the repo) build & pack mydep by running cd mydep && pnpm install && pnpm build - then go back to the root of the project and run rm -rf pnpm-lock.yaml && pnpm i.

The mydep package is very simple:

import { drizzle } from 'drizzle-orm/libsql';
import { createClient } from '@libsql/client';

export async function test(dbURL) {
    const client = createClient({ url: dbURL });
    const db = drizzle(client);
    console.log('db', db);
}

It has the following dependencies:

    "@libsql/client": "0.6.2",
    "drizzle-orm": "0.32.1",

Calling test connects to an SQLite db and then logs the db object to console.

Within the Next.js app/page.tsx we're using that package like this:

import { test } from 'mydep';

export default function Home() {
  test('file:./test-sqlite.db')

  return (
    <p>
      test
    </p>
  );
}

Super simple! External package uses drizzle-orm and @libsql/client to connect to sqlite - that external package is then used within the page.tsx.

When running pnpm dev & opening the page, it works fine. db object is logged to console, and a test-sqlite.db is created.

When running pnpm dev --turbo you will encounter the serverExternalPackages error.

I shouldn't have to install @libsql/client in the consuming project for turbo to work. @libsql/client is a dependency (not peerDependency) of mydep and scoped by pnpm to mydep. It can thus be used by mydep - but we cannot import @libsql/client directly by the consuming project. And this is good! That's how it should be. It's implementation detail of mydep, and in our consuming project, we can only use what mydep exports.

But Turbopack currently does not respect this. It would be a nightmare for package authors to force their users to install their packages' own dependencies

samcx commented 4 weeks ago

Hmm, it seems to not just be that package.

⨯ ./node_modules/.pnpm/@libsql+client@0.6.2/node_modules/@libsql/client/lib-esm/node.js
Package @libsql/client (serverExternalPackages or default list) can't be external
The request @libsql/client matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.

./node_modules/.pnpm/libsql@0.3.19/node_modules/libsql/index.js
Package libsql (serverExternalPackages or default list) can't be external
The request libsql matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.

./node_modules/.pnpm/pino@9.3.1/node_modules/pino/pino.js
Package pino (serverExternalPackages or default list) can't be external
The request pino matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.

./node_modules/.pnpm/pino-pretty@11.2.1/node_modules/pino-pretty/index.js
Package pino-pretty (serverExternalPackages or default list) can't be external
The request pino-pretty matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.

I was able to confirm! I will be sharing this with the :turbopack-spin: team to take a look.

samcx commented 4 weeks ago

Good news, it was already reported internally—we are taking a look!

jmikrut commented 4 weeks ago

Fantastic - thank you all!

khuezy commented 4 weeks ago

Did something happen in canary 114? next dev --turbo works now. And he memory consumption appears to be lower (will need to keep an eye out)

samcx commented 4 weeks ago

@khuezy what version were you using before?

mischnic commented 4 weeks ago

(Responding to https://github.com/vercel/next.js/issues/68805#issuecomment-2289039278)

Due to various implementation detail intricacies in pnpm, that does indeed work because that external package is available even though it wasn't actually declared anywhere as a dependency to pnpm.

To demonstrate how the old (Webpack) behavior could break your app, I modified your reproduction: https://github.com/mischnic/turbopack-externals-repro (commit 571437bb) to trigger a just-as severe or even worse failure mode where the external dependency version might change after building.

mydep has "drizzle-orm": "0.32.1" as a dependency, and the Next.js app itself has the semver-incompatible "drizzle-orm": "0.33.0" (maybe this is unlikely for drizzle-orm itself, but is definitely possible for other dependencies. And this could also occur depending on how your package manager hoists/organizes the node_modules tree). When running pnpm dev or pnpm build && pnpm start (Webpack), this is printed to the terminal.

got version 0.33.0 expected 0.32.1

So now mydep has actually imported drizzle-orm via its own declared dependency, but the coincidentally existing dependency in the project direct. If the other dependency version contains a breaking change, your app is broken.

With growing project size, this becomes more likely because there are simply more dependencies and possibilities for hoisting to not do what you want.

Turbopack fails and tells you what is going wrong:

Package drizzle-orm (serverExternalPackages or default list) can't be external
The package resolves to a different version when requested from the project directory compared to the package requested from the importing module.
Make sure to install the same version of the package in both locations.

(And in the next canary, will also include the offending version numbers: https://github.com/vercel/next.js/pull/68871)

This is why I think that the Turbopack error message is helpful to prevent these kind of (hard to diagnose) problems.


To maybe explain some of the behavior you're seeing with older versions:

Before canary 84, Turbopack didn't externalize dependencies inside node_modules (so if libsql should be external, that would only have worked in user code and not when importing libsql in some other package), now it behaves like Webpack.

AlessioGr commented 4 weeks ago

Hey @mischnic

I see why a version mismatch is an issue and why it makes sense to throw an error there - we do something similar in our project for critical dependencies.

However, I think this might be a separate issue than what we're experiencing here. I see how it makes sense in your modified repro since there are 2 conflicting versions of the dependency installed, but in my repro, there is only a single version of the drizzle-orm / libsql dependency installed. The external package declares it, and the consuming Next.js project does not. So even though there is no dependency version mismatch, Turbo still throws an error.

The underlying error is also different. Yours mentions

The package resolves to a different version when requested from the project directory compared to the package requested from the importing module.

which totally makes sense.

However, our error is

The request could not be resolved by Node.js from the project directory.

No dependency version mismatch

khuezy commented 4 weeks ago

@samcx it may be a mismatch version the others have stated. I had an older version of @libsql/client in one of my packages. edit: nvm, I changed back to an older version of 1 of my packages and it still works... I'm honestly not sure what happened. edit2: canary.112 had the problem, canary.113 and up fixed it.

https://github.com/vercel/next.js/releases/tag/v15.0.0-canary.113 I'm using instrumentation, so perhaps this fixed it: https://github.com/vercel/next.js/pull/68835

jmikrut commented 4 weeks ago

@mischnic your notes make sense, and we understand your reasoning, but we flatly do not think that the current Turbopack functionality is acceptable. (AlessioGr is on my team).

We would rather the onus of mismatching versions be on the developer, so they can be responsible for the risk you've described rather than Turbopack becoming non-functional even in cases where versions match.

It is strange that there are inconsistencies with package managers (dev needs to now think about which package manager they use in order to use Turbopack).

How does your implementation of Webpack currently accomplish this? This all works in Webpack correctly.

The Node ecosystem would require a dependent package based on the context that it was required in. Maybe your current Webpack implementation patches in / uses a workaround to do something similar?

Example, if we install DependencyA which installs and relies on DependencyB, then DependencyA would require DependencyB and it would be sourced from DependencyA's node_modules. Right?

As Turbopack works now, a require is always a top-level require. I think this is where the disconnect is. I also understand the fact that Next.js bundles and the post bundle will indeed technically be a top-level require, but that is an implementation detail that I think Turbopack should solve for.

I really think that if we have to tell people to install all sub-dependencies of our packages in their repo, they will simply be inclined to continue using Webpack rather than Turbopack.

In any case, thank you for your continued insight! We just want to voice our concern and ensure that we know how to proceed here (if we do indeed have to tell end-users of Payload to install our sub-dependencies manually or not).

hisamafahri commented 4 weeks ago

+1 Experiencing the same error with Prisma by following this guide

./node_modules/.pnpm/@prisma+client@5.18.0_prisma@5.18.0/node_modules/@prisma/client/default.js

Package @prisma/client (serverExternalPackages or default list) can't be external
The request @prisma/client matches serverExternalPackages (or the default list), but it can't be external:
The request could not be resolved by Node.js from the project directory.
Packages that should be external need to be installed in the project directory, so they can be resolved from the output files.
Try to install the package into the project directory.
mischnic commented 3 weeks ago

How does your implementation of Webpack currently accomplish this? This all works in Webpack correctly.

Webpack simply doesn't have these error messages, the output is/would be the same.

using Webpack rather than Turbopack.

It's not really a question of Webpack vs Turbopack. It actually depends on the package manager your end-users use:

I've updated https://github.com/mischnic/turbopack-externals-repro to use npm instead of pnpm, and it actually fails with next dev (Webpack) because of implementation detail differences between npm and pnpm. There's now a runtime error:

Error: Cannot find module 'drizzle-orm/version'
Require stack:
- /Users/niklas/Desktop/very-minimal-turbo-bug-repro/.next/server/app/page.js
- /Users/niklas/Desktop/very-minimal-turbo-bug-repro/node_modules/next/dist/server/require.js
- /Users/niklas/Desktop/very-minimal-turbo-bug-repro/node_modules/next/dist/server/load-components.js

This is precisely the situation Turbopack also shown the error, only it's more actionable because you know why it's happening (an external), and the error is shown deterministically at build time.

jmikrut commented 3 weeks ago

Sorry for delay in response today - I have not had the chance to go deep here until just now. Keeping Turbopack working with Payload as it changes is a top priority of ours and I'll try to rapid-fire the best I can.

I've updated https://github.com/mischnic/turbopack-externals-repro to use npm instead of pnpm, and it actually fails with next dev (Webpack) because of implementation detail differences between npm and pnpm. There's now a runtime error:

So in addition to using NPM, your repro actually adds the packages to Webpack externals, which we are in control of in userland and I would be happy to concede that yes, that could break things.

But we're not marking the offending packages as externals in Webpack (pino, @libsql/client, mongodb, etc) so this is not a head-to-head comparison of our issue.

If you remove those lines that you added to your repro from next.config then all is well with Turbopack + npm again - it just breaks with Turbopack + pnpm.

I don't know the difference between marking something as a Webpack external vs. what serverExternalPackages does, but clearly there's something different and it can be demonstrated.

Things used to work great with Turbo prior to canary 84. The fact is that we had no runtime errors prior, but now we do (only with Turbopack). And I understand that Turbo now properly externalizes serverExternalPackages but that means that maybe the offending packages don't need to be listed under that default list any longer for Turbopack (if they were getting compiled and working just fine prior). Could still keep them there for Webpack maybe but they might not need to be added to that list for Turbo.

You are all obviously the experts here (I am out of my league). I don't have strong feelings regarding which mechanism is used to resolve this.

But in the event there are no version conflicts, I REALLY don't want to force my end-users to define dependencies in their own package.json which we already ship for them. So I would be very grateful for a way around that, whatever it is.

mischnic commented 3 weeks ago

So in addition to using NPM, your repro actually adds the packages to Webpack externals, which we are in control of in userland and I would be happy to concede that yes, that could break things.

That was already in the original repro from @AlessioGr, I just had to use drizzle-orm instead because I wasn't able to trigger a version mismatch with libsql: https://github.com/AlessioGr/very-minimal-turbo-bug-repro/blob/f3b3014ecccaa11fb508daa569cbbdb4eea3fb7a/next.config.js#L9 Not sure why they added that in the first place. And Webpack externals are also used in https://unpkg.com/browse/@payloadcms/next@3.0.0-beta.29/dist/withPayload.js (I couldn't find the github repo for that, BTW).

I don't know the difference between marking something as a Webpack external vs. what serverExternalPackages does, but clearly there's something different and it can be demonstrated.

I didn't know this either until today, but Webpack has the counter-intuitive behavior that if an import resolves to a difference version from the project root, Webpack just ignores that serverExternalPackages entry (but it will always respect the externals config): https://github.com/vercel/next.js/blob/0c74267e3d711abb08eb57b0db50f6fb6edb1638/packages/next/src/build/handle-externals.ts#L102-L105

Things used to work great with Turbo prior to canary 84. The fact is that we had no runtime errors prior, but now we do (only with Turbopack).

Have you got some example from that time I could run? If I force Turbopack in canary 84 to not externalize libsql, I just get Error: could not resolve "@libsql/darwin-arm64" into a module in https://github.com/AlessioGr/very-minimal-turbo-bug-repro.

mischnic commented 3 weeks ago

Do you know/remember why you are externalizing drizzle-kit, pino, pino-pretty and graphql?

jmikrut commented 3 weeks ago

I didn't know this either until today, but Webpack has the counter-intuitive behavior that if an import resolves to a difference version from the project root, Webpack just ignores that serverExternalPackages entry (but it will always respect the externals config):

Woah, strange. So that's the discrepancy then.

Have you got some example from that time I could run?

Yep, we can hook you up. So, SQLite is a brand new database adapter that we support and it looks like it's currently incompatible with Turbopack regardless, so to properly test here we'd want to swap out that adapter and use one that we know works (like Postgres / MongoDB). But those will require you to have a database connection to test with. Do you have a preference for which adapter you'd like us to remove SQLite in favor of? Whichever's easier for you to supply a connection string. Then we'll make a repro.

Do you know/remember why you are externalizing drizzle-kit, pino, pino-pretty and graphql?

We're externalizing drizzle-kit because it doesn't need to be used in production and is a large-ish dependency. We probably could bundle it but I think it would slow down compilation a good bit. Our code needs to reference it, but it's not often used in production so it was a deliberate move to try and keep compile times low / serverless functions small.

As for pino and pino-pretty, at one point we had to externalize them because they would break Webpack / Turbopack if bundled. I would love to not externalize them! I can do some checking to see if that is still the case. I remember it was super tough to get them to bundle properly and that was the fix, but maybe that's changed now. Will get back to you on that.

Give us a bit here and I will get back to you. Thanks for the recon!

jmikrut commented 3 weeks ago

OK - we have a repo using Postgres set up that demonstrates the issue.

https://github.com/jmikrut/nextjs-68805-pg

For clarity, I've added our entire withPayload.js next.config plugin into the next.config.mjs file so you can see exactly what it does. I commented out the pino, pino-pretty, drizzle-kit, etc. dependencies (you'll see in the next config). Might not need to have those ones externalized anymore which would be great.

But Turbopack still errors on the pg package, which we install on behalf of the user so they don't have to. Our other database adapters each have packages that we install on behalf of the user, i.e. mongoose and mongodb.

Does this help?

mischnic commented 3 weeks ago

Great, I'll take a look!

SQLite is a brand new database adapter that we support and it looks like it's currently incompatible with Turbopack

You mentioned this in the past as well, can you provide more details? Is it that error where it fails to load the (correct) native binding when including libsql (because it does require(@libsql/{some_dynamic_expression}), or are you seeing something else?

jmikrut commented 3 weeks ago

Yep - we haven't troubleshooted that libsql issue yet. That's a Drizzle / libsql thing I think more than a Payload thing, but the error is exactly what you've mentioned above:

Error: could not resolve "@libsql/darwin-arm64" into a module

It would be great to be able to get to the bottom of that one too! Different issue though. I totally am OK with dodging that one for now. Might even be something that libsql should / could solve, right?

mischnic commented 3 weeks ago

OK - we have a repo using Postgres set up that demonstrates the issue. jmikrut/nextjs-68805-pg

The latest Turbopack in canary 120 compiles that just fine when not making pg external (it's in the default list: https://github.com/vercel/next.js/blob/aa90fe9bb779f0a01b663c19ce3bc5e29dc2c80d/packages/next/src/lib/server-external-packages.json#L43), and Webpack also works just fine.

Turbopack in canary 80 also bundled it, because of the fix for transitive externals that only happened in canary 83.

So we should be able to just remove that one.

(I did that test by removing that package from node_modules/next/dist/lib/server-external-packages.json)


Might even be something that libsql should / could solve, right?

The problem is this very dynamic require, it might be possible to do more code analysis in Turbopack to fix this. But Webpack also throws a build error when not externalizing that.

https://github.com/tursodatabase/libsql-js/blob/bc44c1a08cb3f7e5baaed62af45c25f5b588ccb2/index.js#L3-L23

jmikrut commented 3 weeks ago

Oh nice, so pg no longer causes issues? What about mongoose and mongodb? Should I test again with those? Because those are really the last remaining ones that we need to address!

Yes, I figured the libsql issue would be trickier to solve. I think they have an open issue about this. I will bring it up with them as well.

mischnic commented 3 weeks ago

Oh nice, so pg no longer causes issues?

I'm not sure if I hit all code paths at runtime, but apparently it works.

What about mongoose and mongodb

They also appear to bundle just fine in your example https://github.com/jmikrut/nextjs-68805-pg

I figured the libsql issue would be trickier to solve. I think they have an open issue about this.

I found these https://github.com/tursodatabase/libsql-js/issues/70 https://github.com/tursodatabase/libsql-js/issues/45

seloElo commented 1 week ago

Prisma

Have the same issue as this, I had to de-activate turbopack mode for my project for now