Support Yarn 2 PnP

[simnalamburt]

What: Support Yarn 2 PnP
Why: Plug'n'Play strategy is being supported by Yarn and pnpm. It's much faster than node_modules approach and used by many users.
How: With PnP, you always have to specify all dependencies explicitly. So did I.
Checklist:

• Documentation
• Tests
• Ready to be merged

Reference

• https://webpack.js.org/blog/2020-10-10-webpack-5-release/#changes-to-the-structure

[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/nextauthjs/next-auth/7ybp0vf4y
✅ Preview: https://next-auth-git-fork-simnalamburt-webpack-5.nextauthjs.vercel.app

[balazsorban44]

Wouldn't Next.js include it for us already?

I cannot say I'm entirely sure how or why this is needed. Does that mean that any package being used with Webpack 5 needs to add an additional package? Doesn't seem right.

What am I missing?

[simnalamburt]

@balazsorban44 Sorry, I just checked again and it was issue with PnP not webpack 5.

We don't need to add all packages used with Webpack 5. What we need to do is just specifing required dependencies explicitly. Since "process" dependencies is already required by nextjs, users won't have to download additional dependencies due to this patch.

How to reproduce this error:

1. Download this repository: error-reproduce.bundle.gz

2. gzip -d error-reproduce.bundle.gz
   git clone error-reproduce.bundle
   cd error-reproduce
   yarn
   yarn dev
   

[merceyz]

Wouldn't Next.js include it for us already?

webpack/nextjs sets the process.env values automatically yes, I don't quite get what the dependency here is for

[simnalamburt]

Without explicitly specifying dependencies, you get this error in Plug’n’play mode with Yarn 2 or pnpm

[arcanis]

Next.js happens to add the process polyfill via ProvidePlugin, here:

// Makes sure `process` is polyfilled in client-side bundles (same behavior as webpack 4)
isWebpack5 &&
  !isServer &&
  new webpack.ProvidePlugin({ process: ['process'] }),

As a result, each time you make:

process.env.FOO

Webpack translates it into:

require('process').env.FOO

As such, while you don't have explicit dependencies on process, you have implicit ones. The alternative would be to tweak Next to instead use require.resolve:

// Makes sure `process` is polyfilled in client-side bundles (same behavior as webpack 4)
isWebpack5 &&
  !isServer &&
  new webpack.ProvidePlugin({ process: [require.resolve('process')] }),

This way the resolve dependency would be pre-resolved relative to Next itself, so Webpack wouldn't need to try to resolve it relative to next-auth.

[iaincollins]

Thanks for raising the PR and awareness of the issue, I'm rejecting this PR though.

The process module is a built in function of Node.js and we should not be including it in package.json.

As a built-in function it does not need to be required in Node.js.

If some bundling or package management software isn't compatible with well formed Node.js applications doesn't work unless libraries do that, I suggest taking it up with the maintainers of that software.

[arcanis]

If some bundling or package management software isn't compatible with well formed Node.js applications doesn't work unless libraries do that, I suggest taking it up with the maintainers of that software.

Just to be sure I'm completely clear: this has nothing to do with Yarn, which rightfully reports an unlisted dependency (in either Next or Next-Auth). Because the dependency isn't correctly listed, all package managers can and will generate invalid node_modules layouts. Hoisting issues aren't new in the Node world; Yarn just happens to make it very clear when it happens.

More details and examples in our Rulebook, the reference on what packages must and mustn't do to be portable.

[iaincollins]

process is a built in global of Node.js and does not need to be included with require, as per the Node.js documentation for it which I have linked to above:

The process module doesn't need to be required - it is somewhat literally a wrapper around the currently executing process, and many of the methods it exposes are actually wrappers around calls into core C libraries.

[arcanis]

Right, but this is irrelevant in this case because your code is inside a code that also gets executed inside the browser. For instance, this file in a folder aptly named client is executed within the client, as you can see from the presence of document inside it, which isn't a Node API:

next-auth/src/client/index.js

Lines 25 to 26 in 416d92c

	baseUrl: parseUrl(process.env.NEXTAUTH_URL || process.env.VERCEL_URL).baseUrl,
	basePath: parseUrl(process.env.NEXTAUTH_URL).basePath,

Next HAS to do some transpiling in order to make globals like process work, and that's what's causing this problem, because they transform your code into require('process'). I'm not saying the bug is in next-auth (and that's why the Next.js maintainers have asked you to open an issue on their repo), but don't claim it comes from Yarn.

Frankly, you just have to actually check the generated bundle to see the require calls.

[iaincollins]

Right, but this is irrelevant in this case because your code is inside a code that also gets executed inside the browser. For instance, this file in a folder aptly named client is executed within the client, as you can see from the presence of document inside it, which isn't a Node API:

Only the client file (and code it includes) in this repo is isomorphic and is ever executed in the browser, most of the code isn't and is Node.js only. This PR adds a bunch of require statements to multiple files and adds a third party module called process, which is not the same as using Node.js built in function.

process in this context is an entirely different third party module and is not appropriate as a direct dependency in Package.json or in files that run server side (or, actually in any file). When the client code is run in the browser, it will leverage the global shim for process used by Next.js, which is not the same as the process module referenced here.

If the original report is incorrect and it's working fine with Yarn PnP / pnpm, then it's okay for this to be closed.

If the original report is correct and that it errors with some party package managers but works fine with NPM, then it's okay for this to be closed as the issue is not our problem and can be resolved in those package managers (or avoided, by using NPM).

If there is an error that can be replicated with NPM (e.g. some version of Webpack) I'm very happy to explore our options.

Thanks again for raising the PR @simnalamburt - if you'd like to clarify your experience further with comments please do!

[simnalamburt]

@arcanis Let me fix this issue by patching the Next.js. Thanks :)

[simnalamburt]

FYI, this error does exist and reproducible as I stated in #1034 (comment)

[simnalamburt]

Opened PR in Next.js: vercel/next.js#20971, Thanks!