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

Dependencies with import condition imported as promises #49898

Closed
1 task done
michaldudak opened this issue May 17, 2023 · 9 comments
Closed
1 task done

Dependencies with import condition imported as promises #49898

michaldudak opened this issue May 17, 2023 · 9 comments
Assignees
Labels
linear: next Confirmed issue that is tracked by the Next.js team. locked

Comments

@michaldudak
Copy link
Contributor

michaldudak commented May 17, 2023

Verify canary release

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

Provide environment information

Operating System:
      Platform: win32
      Arch: x64
      Version: Windows 10 Pro
    Binaries:
      Node: 18.16.0
      npm: N/A
      Yarn: N/A
      pnpm: N/A
    Relevant packages:
      next: 13.1.6
      eslint-config-next: N/A
      react: 18.2.0
      react-dom: 18.2.0

Which area(s) of Next.js are affected? (leave empty if unsure)

No response

Link to the code that reproduces this issue

https://github.com/emmatown/test-next-esm

To Reproduce

Run next build in pages-dir, app-dir, and pages-dir-with-app-dir and compare the logs. We're logging the result of import * as x from "../commonjs"; in each case:

Using the pages directory

from client component in pages dir Promise {
  <pending>,
  [Symbol(webpack exports)]: Object [Module] { unitless: [Getter] },
  [Symbol(webpack queues)]: [Function (anonymous)]
}

Note that the imported value is a promise, which is unexpected.

Using the app directory

from server component in app dir Object [Module] { unitless: [Getter] }
from client component in app dir Object [Module] { unitless: [Getter] }

This is the correct behavior.

Using the pages directory with the app directory present

from client component in pages dir Object [Module] { unitless: [Getter] }

This is also correct.

Describe the Bug

The issue appeared in the MUI Core repository when we updated Emotion to the latest version (11.11.0). This version of Emotion includes an addition of the import condition in its package.json (see https://github.com/emotion-js/emotion/pull/3029/files#diff-1f344ac391eeecc21ec0f01fb07430a47f4b80d20485c125447d54c33c4bbfc4R16).

It causes the next build output (a built page inside .next/server/pages) to change from

/***/ 553139:
/***/ ((module) => {

module.exports = require("@emotion/react");;

/***/ }),

to

/***/ 553139:
/***/ ((module) => {

module.exports = import("@emotion/react");;

/***/ }),

which, in turn causes dependent imports to be dynamic (= return a promise instead of the actual value), breaking our app.

We have also noticed a similar bug when installing the Floating UI package (which also has the import condition in its package.json (mui/material-ui#35914))

Emotion issue for reference: emotion-js/emotion#3032 (comment)

Expected Behavior

Import dependencies without wrapping them in promises when the import condition is used.

Which browser are you using? (if relevant)

No response

How are you deploying your application? (if relevant)

No response

NEXT-1565

@Andarist
Copy link
Contributor

friendly 🏓 @huozhi

@huozhi huozhi added linear: next Confirmed issue that is tracked by the Next.js team. and removed bug Issue was opened via the bug report template. labels Aug 26, 2023
@huozhi
Copy link
Member

huozhi commented Aug 26, 2023

Tried with latest canary the issue is still preserved, will investigate on that 🙏

@huozhi
Copy link
Member

huozhi commented Sep 27, 2023

Is there any case that you have to remain a "commonjs.js" to re-export the esm bundle? For app directory it bundles everything, and we disabled the esm resolving to prefer cjs. If you're using esm import/export synatx it will handle properly.

@Andarist
Copy link
Contributor

Andarist commented Sep 27, 2023

Is there any case that you have to remain a "commonjs.js" to re-export the esm bundle?

It was likely just an old setup that resulted in this but I imagine people could try to write code like this to get direct access to __dirname or to conditionally load modules synchronously using require.

I feel like this issue is more of a "shouldn't this work?" rather than "we can't live without it".

For app directory it bundles everything, and we disabled the esm resolving to prefer cjs.

I don't quite follow this. Some examples of this behavior would be appreciated.

@huozhi
Copy link
Member

huozhi commented Sep 27, 2023

It seems any dual package that would have this problem.
For pages only: if a package is allowed to be resolved as both esm and cjs, then it webpack will try to resolve it as esm, it will effect the modules that import it to become async module. Then you "require" that async module, so it become module.exports = async module, so you're seeing promise.

For app router: It bundles everything, so external dependencies can be extracted, then the external esmodule could be handled as properly.

For pages + app router:
I think here introduced the confusion, that the one imported to pages should still be handled as async module. Might potentionally change this. Then they will be handled separately in pages and app router.

@huozhi
Copy link
Member

huozhi commented Oct 10, 2023

With change from #56532 we keep the original external resolving logic, so it gonna be aligned between nextjs pages app (with app directort) and nextjs pages app (without app directory). But the result will still remain as promise due to the reason explained above.

@huozhi
Copy link
Member

huozhi commented Oct 12, 2023

@Andarist @michaldudak the alignment is landed in v13.5.5-canary.9, I'd like to close this for now. Since pages dir is not bundled so we cannot handle the esm -> cjs -> esm case for pages dir, bundler is not aware if the deepest esm dependency is sync and will treat it as async by default.

@huozhi huozhi closed this as completed Oct 12, 2023
@michaldudak
Copy link
Contributor Author

michaldudak commented Oct 13, 2023

Thanks for taking a look at this! The fix doesn't solve our problem in MUI docs, but we were able to work around it by setting esmExternals: false.

@github-actions
Copy link
Contributor

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
linear: next Confirmed issue that is tracked by the Next.js team. locked
Projects
None yet
Development

No branches or pull requests

3 participants