Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused Server-Only Dynamic Import Inside typeof window === 'undefined' Causes Error #36514

Open
1 task done
karlhorky opened this issue Apr 27, 2022 · 10 comments
Open
1 task done
Labels
SWC Related to minification/transpilation in Next.js.

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Apr 27, 2022

Verify canary release

  • I verified that the issue exists in Next.js canary release

Provide environment information

$ npx --no-install next info

    Operating System:
      Platform: linux
      Arch: x64
      Version: #112-Ubuntu SMP Thu Feb 3 13:50:55 UTC 2022
    Binaries:
      Node: 14.18.1
      npm: 6.14.15
      Yarn: 1.22.17
      pnpm: N/A
    Relevant packages:
      next: 12.1.6-canary.9
      react: 18.0.0
      react-dom: 18.0.0

What browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

Describe the Bug

Hi there! First of all, thanks for your continued effort on Next.js! 🙌 Really amazing framework.

Importing a client-only function in a Next.js page causes bundling of the server-only code contained within the typeof window === 'undefined' check:

// pages/index.js
import { usedClientExport } from "../util/shared";

export default function IndexPage() {
  usedClientExport();
  return <div>Hello World</div>;
}
// util/shared.js
export async function unusedServerExport() {
  if (typeof window === "undefined") {
    await import("fs");
  }
}

export function usedClientExport() {
  console.log("yay");
}

Error:

Screen Shot 2022-04-27 at 16 36 32

./util/shared.js:3:10
Module not found: Can't resolve 'fs'
  1 | export async function unusedServerExport() {
  2 |   if (typeof window === "undefined") {
> 3 |     await import("fs");
    |          ^
  4 |   }
  5 | }
  6 | 

Sandbox: https://codesandbox.io/s/recursing-lalande-k3w986?file=/util/shared.js

cc @balazsorban44 @sokra

Expected Behavior

  1. Any unused functions (in my example, unusedServerExport) are removed using tree shaking or dead code elimination and not included in the client bundle
  2. Any code in typeof window === 'undefined' is not included in the client bundle
  3. No errors appear for any code that will not be included in the bundle (in either of 1 or 2)

To Reproduce

https://codesandbox.io/s/recursing-lalande-k3w986?file=/util/shared.js

‼️ This also occurs when using Babel instead of SWC:

https://codesandbox.io/s/unruffled-fire-jrkz7x?file=/pages/index.js (also here on StackBlitz: https://stackblitz.com/edit/nextjs-wmguir?file=pages%2Findex.js)

Potentially Related Issues

@karlhorky karlhorky added the bug Issue was opened via the bug report template. label Apr 27, 2022
@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 27, 2022

Workaround

A (very weird) workaround is to omit the await before the dynamic import():

export async function unusedServerExport() {
  if (typeof window === "undefined") {
-    await import("fs");
+    import("fs").then((m) => {
+      console.log("imported", m);
+    });
  }
}

Sandbox: https://codesandbox.io/s/empty-https-y9ws41?file=/util/shared.js:0-163

Screen Shot 2022-04-27 at 16 49 28

@karlhorky karlhorky changed the title Tree Shaking / Dead Code Elimination Bundles Dynamic Import Inside typeof window check Tree Shaking / Dead Code Elimination Bundles Server-Only Dynamic Import Inside typeof window check Apr 27, 2022
@karlhorky karlhorky changed the title Tree Shaking / Dead Code Elimination Bundles Server-Only Dynamic Import Inside typeof window check Server-Only Dynamic Import Inside typeof window check Bundled in Client Apr 27, 2022
@karlhorky karlhorky changed the title Server-Only Dynamic Import Inside typeof window check Bundled in Client Unused Server-Only Dynamic Import Inside typeof window === 'undefined' Causes Error Apr 27, 2022
@timneutkens
Copy link
Member

Seems the main difference between the Babel/SWC approach is that the Babel transform we used to transform typeof window actually evaluates the expression and inlines the result whereas with the SWC transform it doesn't evaluate it:

// util/shared.js
export function unusedServerExport() {
  if (typeof window === "undefined") {
    import("fs");
  }
}

export function usedClientExport() {
  console.log("yay");
}

Result with SWC:

// util/shared.js
export function unusedServerExport() {
    if ("object" === "undefined") {
        import("fs");
    }
}
export function usedClientExport() {
    console.log("yay");
}

Result with next/babel:

// util/shared.js
export function unusedServerExport() {
    if (false) {
        import("fs");
    }
}
export function usedClientExport() {
    console.log("yay");
}

@timneutkens timneutkens added SWC Related to minification/transpilation in Next.js. kind: bug and removed bug Issue was opened via the bug report template. labels Apr 28, 2022
@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 28, 2022

Ok, but two things:

  1. In your code above, there is no await in front of the import(), which is what causes the Can't resolve 'fs' error described in the original post
  2. The problem also appears when using Babel instead of SWC https://codesandbox.io/s/unruffled-fire-jrkz7x?file=/pages/index.js (also here on StackBlitz: https://stackblitz.com/edit/nextjs-wmguir?file=pages%2Findex.js)

Screen Shot 2022-04-28 at 12 41 47

Maybe there are two different bugs here...

@timneutkens
Copy link
Member

Note that Next.js does not claim to even tree-shake these, we only tree shake get(Static/ServerSide)Props and getStaticPaths. For typeof window we only transform this to 'object' or 'undefined' and then Terser/swcMinify remove the code during minification. In this case webpack will still try to resolve/bundle all import() calls because well, they're in your code even when transformed. I'm not sure why a certain case you're referring to above would exclude it, maybe that's a bug even.

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 28, 2022

So am I understanding correctly that it's impossible to write a shared util file like this with Next.js?

By "shared util" I mean a file including:

  1. Exported backend functions (depending internally on Node.js deps such as fs, with dynamic import)
  2. Exported frontend functions (depending internally on frontend-only concepts like localStorage)
  3. Importing a frontend export in a shared component in pages/ - outside of gSSP / gSP

That would be too bad! It's nice to group functions for both server and client in the same util file...

@karlhorky
Copy link
Contributor Author

karlhorky commented Apr 28, 2022

Or is it the current recommendation of the Next.js team to use the isServer and config.node webpack configuration to hack around issues like this?

Seems a bit unusual to have to do this, especially when the code is not being used.

module.exports = {
  webpack: (config, { isServer }) => {
    if (!isServer) {
      config.node = {
        fs: 'empty'
      };
    }
    return config;
  }
};

As seen in resources like this or this

@timneutkens
Copy link
Member

So am I understanding correctly that it's impossible to write a shared util file like this with Next.js?

By "shared util" I mean a file including:

  1. Exported backend functions (depending internally on Node.js deps such as fs, with dynamic import)
  2. Exported frontend functions (depending internally on frontend-only concepts like localStorage)
  3. Importing a frontend export in a shared component in pages/ - outside of gSSP / gSP

That would be too bad! It's nice to group functions for both server and client in the same util file...

Correct, it's a compilation limitation. With Server Components there will be a file-level convention to mark something as server-only.

Related: #16153 (comment)

@karlhorky
Copy link
Contributor Author

Ok, maybe the .server.js / .client.js convention is something to adopt for non-React code then too, at least with Next.js apps, for now.

We'll also try out the hacky-feeling workaround to see if this is any more ergonomic.

@stephenrobertsuk
Copy link

stephenrobertsuk commented Jun 9, 2022

@karlhorky I also hit this problem when trying to create an isomorphic utility and am trying to work around it.



So far I have managed by dynamically importing the module (which contains server-only code and imports) within a block which only executes in the server context (dependent on a typeof window check). Also, adding this module to webpack externals if !isServer. This too feels a bit hacky in comparison to how this type of dead code may have been eliminated from the client bundle previously.

@pirtlj
Copy link

pirtlj commented Aug 17, 2022

Also we need to ensure tree shaking so all the isomorphic server specific code does not get included in the client bundle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SWC Related to minification/transpilation in Next.js.
Projects
None yet
Development

No branches or pull requests

5 participants