msw not being tree shaken from client bundle (dead code elimination bug)

[ismay]

What example does this report relate to?

with-msw

What version of Next.js are you using?

10.2.3

What version of Node.js are you using?

14.16.1

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

Vercel

Describe the Bug

next build isn't eliminating this require from the client bundle:

// assuming this evaluates to false for production build
if (process.env.NEXT_PUBLIC_API_MOCKING === 'enabled') {
  require('../mocks')
}

It bundles msw for the client bundle, even when the require is contained in an if block that evaluates to false during build time (env var to string comparison). This is reproducible with a slight modification to the with-msw example.

Expected Behavior

I expect next build to drop this code from the client bundle:

if (false) {
  require('../mocks')
}

and not include any of the code imported through the call to require.

To Reproduce

First off:

1. Check out https://github.com/vercel/next.js repo, cd to the with-msw example.
2. Remove the .env.production file (we don't want to mock in production and want to drop the ../mocks require)
3. Run npm install

Reproduce the bug

This build will (erroneously) include msw in the client bundle:

1. After following the above steps, run next build
2. You'll get the following output:

> npm run build

> with-msw@1.0.0 build
> next build

info  - Using webpack 5. Reason: no next.config.js https://nextjs.org/docs/messages/webpack5
info  - Checking validity of types  
info  - Creating an optimized production build  
info  - Compiled successfully
info  - Collecting page data  
info  - Generating static pages (2/2)
info  - Finalizing page optimization  

Page                             Size     First Load JS
┌ λ /                            479 B           112 kB
├   /_app                        0 B             112 kB
└ ○ /404                         3.06 kB         115 kB
+ First Load JS shared by all    112 kB
  ├ chunks/framework.64eb71.js   42 kB
  ├ chunks/main.71948a.js        19.4 kB
  ├ chunks/pages/_app.1a485f.js  49.5 kB
  └ chunks/webpack.189c53.js     994 B

λ  (Server)  server-side renders at runtime (uses getInitialProps or getServerSideProps)
○  (Static)  automatically rendered as static HTML (uses no initial props)
●  (SSG)     automatically generated as static HTML + JSON (uses getStaticProps)
   (ISR)     incremental static regeneration (uses revalidate in getStaticProps)

Build expected bundle

This build is what I would actually expect next to build. It does not include msw in the client bundle. We're simulating the dead code elimination here by completely removing the require call.

1. Remove lines 1 - 3 from the with-msw example's ./pages/_app.js (so remove the conditional msw require)
2. Run next build
3. You'll get the following output:

> n run build

> with-msw@1.0.0 build
> next build

info  - Using webpack 5. Reason: no next.config.js https://nextjs.org/docs/messages/webpack5
info  - Checking validity of types  
info  - Creating an optimized production build  
info  - Compiled successfully
info  - Collecting page data  
info  - Generating static pages (2/2)
info  - Finalizing page optimization  

Page                             Size     First Load JS
┌ λ /                            479 B          63.4 kB
├   /_app                        0 B              63 kB
└ ○ /404                         3.06 kB          66 kB
+ First Load JS shared by all    63 kB
  ├ chunks/framework.64eb71.js   42 kB
  ├ chunks/main.71948a.js        19.4 kB
  ├ chunks/pages/_app.6149d5.js  544 B
  └ chunks/webpack.2a5a41.js     952 B

λ  (Server)  server-side renders at runtime (uses getInitialProps or getServerSideProps)
○  (Static)  automatically rendered as static HTML (uses no initial props)
●  (SSG)     automatically generated as static HTML + JSON (uses getStaticProps)
   (ISR)     incremental static regeneration (uses revalidate in getStaticProps)

So as you see, the bundle is a lot smaller. The only change was us removing the conditional msw require. So it seems that next build isn't eliminating the conditional require from the client bundle. This applies to more than just the with-msw example, but that example is an easy way to reproduce this.

I would expect next build (and webpack 5) to drop msw from the client bundle when the require is contained in an if block that evaluates to false during build time.

[dasblitz]

Can confirm that this is happening with webpack 4 too.

[ismay]

In the meantime, for anyone encountering this, you can manually eliminate msw from the browser bundle by adding this to your package.json:

  "browser": {
    "./.msw": false
  },

Make sure you replace ./.msw with the path to the msw mocks that you want to omit for the browser bundle (this is relative to package.json). This path should resolve to the same path that's in the conditional require (so the one you want to omit from the bundle).

See:

• https://arunoda.me/blog/ssr-and-server-only-modules
• https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module
• https://github.com/ismay/superwolff/pull/63

This is just a temporary workaround, ideally the regression would still be fixed in the next.js build process.

[lukasmoellerch]

Setting the env value of NEXT_PUBLIC_API_MOCKING in .env.production to something other than "enabled" results in the import successfully being removed. The issue here is that when the env variable is not defined the process.env. NEXT_PUBLIC_API_MOCKING expression isn't replaced as an env variable and only the process define is applied. The code thus compiles to require(/* process mock */).env.NEXT_PUBLIC_API_MOCKING which doesn't statically evaluate to false.

I initially wanted to work on a fix for this, but I think the behavior of next.js is sensible here, the only option I see would be to somehow replace all env vars which are not defined with undefined.

[ismay]

Thanks for taking a look at this @lukasmoellerch. So the reason that I called this a regression is that in the linked issue here: mswjs/msw#359 (comment), the maintainer of msw verified that next was correctly removing msw. This was verified with the code from the next.js with-msw example.

Currently it no longer does that. So what I'm wondering is: what is the intended behaviour for the next.js bundling process? My expectation would be that the way it worked when @kettanaito checked it, is the way it's supposed to work (I could not verify this in the docs though).

If that is true, then even if the behaviour is sensible, it's a regression. Unless it's been announced as a breaking change, but I haven't seen that mentioned. Let me know what you think.

[lukasmoellerch]

Hi - in mswjs/msw#359 (comment) the maintainer mentions that the env variable was set to disabled for the test:

Note: You have to set the NEXT_PUBLIC_API_MOCKING environmental variable to "disabled" in your .env file, otherwise you are bundling a production app with the mocking enabled, which intentionally includes msw in the build.

If this is the case the module is indeed correctly excluded from the bundle. I also couldn't reproduce it with a removed env file with several old next.js versions.

Furthermore it seems like next.js intentionally doesn't default process.env.NOT_DEFINED to undefined as they have an error page for that case: https://nextjs.org/docs/messages/missing-env-value. Currently process.env already evaluates to {} in some cases, but due to terser/terser#584 terser doesn't simplify process.env.NOT_DEFINED to undefined and this would have to be done manually. If it is intended that env vars default to undefined (and enable tree-shaking in cases like this one) I'd be happy to work on an implementation.

[ismay]

Hi - in mswjs/msw#359 (comment) the maintainer mentions that the env variable was set to disabled for the test:

Ah I see, I had missed that.

Furthermore it seems like next.js intentionally doesn't default process.env.NOT_DEFINED to undefined as they have an error page for that case: https://nextjs.org/docs/messages/missing-env-value. Currently process.env already evaluates to {} in some cases, but due to terser/terser#584 terser doesn't simplify process.env.NOT_DEFINED to undefined and this would have to be done manually.

Yeah in that case it seems like it's not a regression and I agree with your initial conclusion. Seems like the build process is working as intended. So maybe then we could mention this behaviour explicitly in the docs, it's a little different than what I expected (might just be me). Also, maybe we could add a note to the with-msw example, just so that it's clear to people what they should do if they want to omit msw from the production bundle.

And then if replacing undefined environment vars with undefined is something others like as well, we could deal with that separately in another issue. I'm fine with the solutions from the previous paragraph though, as long as the behaviour is clearly documented.

What do you think?

[lukasmoellerch]

I'd agree that a note about this behavior in the env var docs would make sense, possibly after the so your various NEXT_PUBLIC_ envs need to be set when the project is built. - maybe something like "Please note that environment variables have to be defined in all environments, this means that you shouldn't rely on process.env.NOT_DEFINED evaluating to undefined. Next.js will notify you in dev mode if you are using environment variable which isn't defined". I don't think that an additional comment in the example would be necessary as it's already mentioned:

To disable MSW for a specific environment, change the environment variable value in the file corresponding to the environment from enabled to disabled.

In general I'd prefer the current behavior that undefined env vars aren't supported, but it might be possible to detect such misconfigurations in production builds. If you really want an environment variable to default to undefined it should be possible to achieve that using next.config.js.

[ismay]

I'd agree that a note about this behavior in the env var docs would make sense, possibly after the so your various NEXT_PUBLIC_ envs need to be set when the project is built. - maybe something like "Please note that environment variables have to be defined in all environments, this means that you shouldn't rely on process.env.NOT_DEFINED evaluating to undefined. Next.js will notify you in dev mode if you are using environment variable which isn't defined".

Yeah exactly, something like that seems nice 👍. Though I guess they don't have to be defined. It's just that if you want dead code elimination based on a comparison to an env var you need them to be defined, as otherwise the comparison can't be evaluated at build time.

I don't think that an additional comment in the example would be necessary as it's already mentioned:

Yeah that seems clear, given that it would also be in the next.js docs if we add something like the above.

In general I'd prefer the current behavior that undefined env vars aren't supported, but it might be possible to detect such misconfigurations in production builds.

Yeah, I don't mind the current behaviour either. And notifying the user about it seems user-friendly 👍.

[lukasmoellerch]

Yeah exactly, something like that seems nice 👍. Though I guess they don't have to be defined. It's just that if you want dead code elimination based on a comparison to an env var you need them to be defined, as otherwise the comparison can't be evaluated at build time.

I see your point, but I am not sure whether it should be stated in the docs like that. The way I see it the use of undefined env vars is unsupported. It's not like this couldn't be implemented in next.js, but it seems like a conscious decision not to support it. Therefore I am not sure whether the docs should define the exact behavior or just the say that it isn't supported.

[ismay]

The way I see it the use of undefined env vars is unsupported. It's not like this couldn't be implemented in next.js, but it seems like a conscious decision not to support it. Therefore I am not sure whether the docs should define the exact behavior or just the say that it isn't supported.

Yeah I see what you mean. Yeah I think a note along the lines of what you suggested here: #25607 (comment) should be good enough.

[kettanaito]

In this light, I'm also sharing #43284, where using a dynamic import presently results in a race condition between the import itself and any initial load requests the app may perform. I wonder why we've migrated from using require(), which is sync. Was it because of this tree-shaking issue?

[balazsorban44]

Going to close this in favor of #43284. In short, require is not tree-shakeable.

[github-actions]

This closed issue has been automatically locked because it had no new activity for a month. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.