PnP breaks the externals detection

[arcanis]

Bug report

Describe the bug (tl;dr)

Using resolve through a prebuilt binary instead of the regular one prevents it from resolving files through PnP, causing it to bake all the third-party files into the SSR bundle:

https://github.com/zeit/next.js/blob/canary/packages/next/build/webpack-config.ts#L9

A derived bug is that contexts & hooks don't work in SSR mode, because they expect the React instance to be unique across the Node resolution and the Webpack resolution (which isn't the case since react and react-dom aren't externals anymore).

Describe the bug

I noticed that when operating under PnP, Next causes React to crash when using hooks (with this error). The problems stems from the following line:

https://github.com/zeit/next.js/blob/e9ffb41462beff7bed9e27717e9558d42f48d17b/packages/next/build/webpack-config.ts#L317-L323

What should have happened is this. This assumes the SSR mode, please feel free to correct me:

• Next (the Node process) generates a Webpack bundle for all/some of the routes.
• Then it requires react-dom/server, which requires react. So far so good.
• Then it requires the Webpack bundle, and triggers a build for the routes it contains.
• Then Webpack page requires react. Since it got marked as being stored within a node_modules directory, it should have been marked as external, and thus would have been required through the Node resolution, at runtime, hence sharing the same instance as the rest of the server.

So what prevents this from working?

• In order to support PnP, resolve uses a "pseudo-hook". It's a specific file that, when required, a resolver wrapper is allowed to replace by whatever it wants (with the idea that it will change the default options depending on the context). The PR that introduced it is here: Implements a "normalize-options" pseudo-hook browserify/resolve#174

• For some reason, Next is compiling resolve through ncc. This causes the file I referenced to disappear entirely, and thus PnP cannot hook itself into it without help.

Potential fixes

From the best to the less practical:

• Next could maybe stop precompiling resolve? I'm not sure why it works that way (cc @timneutkens, ncc resolve and arg #6771)

• Next could just use createRequire to get an handle on require.resolve from the perspective of a particular location on the disk, which would work in both contexts (maybe there isn't a need for resolve after all?).

• Next could just call directly the PnP API when operating under PnP (if (process.versions.pnp) require('pnpapi').resolveRequest(request).includes('node_modules') or similar).

• Next could explicitly pass the PnP options to resolve to compensate for Yarn not being able to inject the right settings. They are defined here.

• Next could use enhanced-resolve with the actual Webpack resolution configuration used by Webpack (which would then include PnpWebpackPlugin). But frankly it might require a hefty refactoring, probably not the most practical solution in the lot.

• resolve could just detect PnP by default without us having to hook into it. I'll open an issue to discuss it, but it might take a while before a consensus is reached.

Screenshots

[timneutkens]

Next could maybe stop precompiling resolve? I'm not sure why it works that way (cc @timneutkens, #6771)

Resolve ships their test suite as part of the package. Which we're not comfortable with doing, so we compile the lib into a single file.

Next could just use createRequire to get an handle on require.resolve from the perspective of a particular location on the disk, which would work in both contexts (maybe there isn't a need for resolve after all?).

Seems like this was added in Node.js 12+ so it doesn't cover our minimum required Node.js version, maybe that tradeoff is fine for PnP users though 🤔

Next could just call directly the PnP API when operating under PnP (if (process.versions.pnp) require('pnpapi').resolveRequest(request).includes('node_modules') or similar).

We could potentially do this if it doesn't run for non-PnP users 👍

[ljharb]

Why would shipping the test suite impact anyone? None of the tests would show up in a browser bundle, since nothing consumes them.

[balazsorban44]

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.