From cb0e21f079896a267e2c7ae2fe57340066ecda7a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Mon, 30 Jan 2023 15:08:22 +0000 Subject: [PATCH 1/4] fix(nextjs): Don't modify require calls --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 3742724ae4fb..25628af3e0d7 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -153,7 +153,7 @@ async function wrapUserCode( // People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to // handle that correctly so we let a plugin to take care of bundling cjs exports for us. commonjs({ - transformMixedEsModules: true, + strictRequires: true, sourceMap: true, }), ], From 527d9cccf58283397ed80ac6c96c54aa0019c92c Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 31 Jan 2023 09:33:23 +0000 Subject: [PATCH 2/4] Ignore require statements in common js plugin --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 25628af3e0d7..571b5a7c8fe2 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -153,8 +153,14 @@ async function wrapUserCode( // People may use `module.exports` in their API routes or page files. Next.js allows that and we also need to // handle that correctly so we let a plugin to take care of bundling cjs exports for us. commonjs({ - strictRequires: true, sourceMap: true, + strictRequires: true, // Don't hoist require statements that users may define + ignore() { + // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. + // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. + // (Also, modifying require may break user code) + return true; + }, }), ], From 808f0c4673e0da7c9a051044f18fd487eb48ae50 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 31 Jan 2023 09:43:02 +0000 Subject: [PATCH 3/4] Don't break dynamic requires --- packages/nextjs/src/config/loaders/wrappingLoader.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/nextjs/src/config/loaders/wrappingLoader.ts b/packages/nextjs/src/config/loaders/wrappingLoader.ts index 571b5a7c8fe2..1e7aac4b7c8a 100644 --- a/packages/nextjs/src/config/loaders/wrappingLoader.ts +++ b/packages/nextjs/src/config/loaders/wrappingLoader.ts @@ -155,6 +155,7 @@ async function wrapUserCode( commonjs({ sourceMap: true, strictRequires: true, // Don't hoist require statements that users may define + ignoreDynamicRequires: true, // Don't break dynamic requires and things like Webpack's `require.context` ignore() { // We want basically only want to use this plugin for handling the case where users export their handlers with module.exports. // This plugin would also be able to convert any `require` into something esm compatible but webpack does that anyways so we just skip that part of the plugin. From edf39856e9fbf028a4412be8ac4093d285dfdf9f Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 31 Jan 2023 10:12:00 +0000 Subject: [PATCH 4/4] Add integration tests --- .../test/integration/pages/api/requireTest.ts | 14 ++++++++ .../test/server/cjsApiEndpoints.test.ts | 33 +++++++++++++++++++ .../integration/test/server/utils/throw.js | 1 + 3 files changed, 48 insertions(+) create mode 100644 packages/nextjs/test/integration/pages/api/requireTest.ts create mode 100644 packages/nextjs/test/integration/test/server/utils/throw.js diff --git a/packages/nextjs/test/integration/pages/api/requireTest.ts b/packages/nextjs/test/integration/pages/api/requireTest.ts new file mode 100644 index 000000000000..cb0674d28c42 --- /dev/null +++ b/packages/nextjs/test/integration/pages/api/requireTest.ts @@ -0,0 +1,14 @@ +import { NextApiResponse, NextApiRequest } from 'next'; + +if (process.env.NEXT_PUBLIC_SOME_FALSE_ENV_VAR === 'enabled') { + require('../../test/server/utils/throw'); // Should not throw unless the hoisting in the wrapping loader is messed up! +} + +const handler = async (_req: NextApiRequest, res: NextApiResponse): Promise => { + require('@sentry/nextjs').captureException; // Should not throw unless the wrapping loader messes up cjs imports + // @ts-ignore + require.context('.'); // This is a webpack utility call. Should not throw unless the wrapping loader messes it up by mangling. + res.status(200).json({ success: true }); +}; + +module.exports = handler; diff --git a/packages/nextjs/test/integration/test/server/cjsApiEndpoints.test.ts b/packages/nextjs/test/integration/test/server/cjsApiEndpoints.test.ts index 510a8c917e4e..fc0ae186f64b 100644 --- a/packages/nextjs/test/integration/test/server/cjsApiEndpoints.test.ts +++ b/packages/nextjs/test/integration/test/server/cjsApiEndpoints.test.ts @@ -66,4 +66,37 @@ describe('CommonJS API Endpoints', () => { success: true, }); }); + + it('should not mess up require statements', async () => { + const env = await NextTestEnv.init(); + const route = '/api/requireTest'; + const url = `${env.url}${route}`; + + const wrappedEnvelope = await env.getEnvelopeRequest({ + url, + envelopeType: 'transaction', + endServer: false, + }); + + expect(wrappedEnvelope[2]).toMatchObject({ + contexts: { + trace: { + op: 'http.server', + status: 'ok', + tags: { 'http.status_code': '200' }, + }, + }, + transaction: `GET ${route}`, + type: 'transaction', + request: { + url, + }, + }); + + const response = await env.getAPIResponse(url); + + expect(response).toMatchObject({ + success: true, + }); + }); }); diff --git a/packages/nextjs/test/integration/test/server/utils/throw.js b/packages/nextjs/test/integration/test/server/utils/throw.js new file mode 100644 index 000000000000..0e37a4135be4 --- /dev/null +++ b/packages/nextjs/test/integration/test/server/utils/throw.js @@ -0,0 +1 @@ +throw new Error('I am throwing');