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

'concurrentFeatures' breaks environment detect logic in @emotion/cache #31678

Closed
KotoriK opened this issue Nov 22, 2021 · 10 comments
Closed

'concurrentFeatures' breaks environment detect logic in @emotion/cache #31678

KotoriK opened this issue Nov 22, 2021 · 10 comments
Labels
bug Issue was opened via the bug report template.

Comments

@KotoriK
Copy link

KotoriK commented Nov 22, 2021

What version of Next.js are you using?

12.0.4

What version of Node.js are you using?

15.4.0

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

next dev

Describe the Bug

Stack

Exception: webpack://_N_E/./node_modules/@emotion/cache/dist/emotion-cache.browser.esm.js?:216
    var ssrStyles = document.querySelectorAll("style[data-emotion]:not([data-s])"); // get SSRed styles out of the way of React's hydration
                    ^

ReferenceError: document is not defined
  at createCache (webpack://_N_E/./node_modules/@emotion/cache/dist/emotion-cache.browser.esm.js?:216:21)
    at eval (webpack://_N_E/./node_modules/@mui/material/node_modules/@mui/styled-engine/StyledEngineProvider/StyledEngineProvider.js?:19:73)
    at Module../node_modules/@mui/material/node_modules/@mui/styled-engine/StyledEngineProvider/StyledEngineProvider.js (/Project/.next/server/pages/index.js:5857:1)
    at __webpack_require__ (/Project/.next/server/middleware-ssr-runtime.js:37:33)
    at fn (/Project/.next/server/middleware-ssr-runtime.js:288:21)
    at eval (webpack://_N_E/./node_modules/@mui/material/node_modules/@mui/styled-engine/StyledEngineProvider/index.js?:5:79)
    at Module../node_modules/@mui/material/node_modules/@mui/styled-engine/StyledEngineProvider/index.js (/Project/.next/server/pages/index.js:5868:1)
    at __webpack_require__ (/Project/.next/server/middleware-ssr-runtime.js:37:33)
    at fn (/Project/.next/server/middleware-ssr-runtime.js:288:21)
    at eval (webpack://_N_E/./node_modules/@mui/material/node_modules/@mui/styled-engine/index.js?:12:79)

Possible Reason

@emotion/cache uses two logics to solve SSR problem.
First is using typeof document:
https://github.com/emotion-js/emotion/blob/57be9e8cb20313bd2ed297a39c41ca0f0ca37ea8/packages/cache/src/index.js#L23

Another is using browser field in package.json, it will hint build tools to use specific build for browser, in which the first logic is removed, assuming it will only be used in browser.
https://github.com/emotion-js/emotion/blob/57be9e8cb20313bd2ed297a39c41ca0f0ca37ea8/packages/cache/package.json#L7-L10

These logics make this package works well with Next.js, but when concurrentFeatures:true, Next.js will load the build for browser, which did not include the first logic, and then cause ReferenceError while calling DOM API.

Expected Behavior

IMO Nextjs should follow load hints since Package Authors may make assumption like @emotion/cache does.

To Reproduce

this bug happened when I tried to turn concurrentFeatures on with @mui/material@5.1.1
https://codesandbox.io/s/nextjs-concurrent-bug-m0fg7

@KotoriK KotoriK added the bug Issue was opened via the bug report template. label Nov 22, 2021
@huozhi
Copy link
Member

huozhi commented Nov 22, 2021

Currently, when you enable experimental flag concurrentFeatures as true, it will use the "custom web rumtime". That's the one which we polyfilled some browser runtime API and pack code as web bundle to run there. This is now supported both in local development and production if you deployed to vercel, as it's still "experimental". This requires library to be better compatible across environment for its browser bundle. In short, nonisomorphic libraries are not supported now.

We're working on the next step to provide user more flexibility to try it with node.js libs with concurrentFeatures is enabled. Fully supporting it on production might be longer. In short term it's kind a trade-off that you might not easily bring node runtime code to streaming. IMO it would be nicer to make the library more cross-env compatible if it could be.

@Andarist
Copy link
Contributor

@huozhi we are doing our best in Emotion to prepare bundles specific to a particular environment. It seems that the bundler here is configured to pick up the browser field and the corresponding file in Emotion contains unguarded references to document.

Webpack docs dont mention if a DOM env can be assumed or not: https://webpack.js.org/guides/package-exports/#target-environment . However this has worked for quite a long time and i’ve seen a lot of libraries assuming a DOM env in their browser files.

I think that a guidance on the webpack side of things should be given here and this should be clarified. If browser doesnt assume a DOM env then we should introduce a dom condition to webpack (and the appropriate file to Emotion). Additional questions - wouldnt a worker condition work here? This could assume browser-like but no DOM env, right?

@huozhi
Copy link
Member

huozhi commented Nov 28, 2021

@Andarist Thanks for the input! We're also thinking of adding some condition to pick up the worker like bundle instead of browser one with DOM env.

Wonder it could be nice to resolve worker field in this web runtime since webpack has already supported and I think we're able to configure it on next.js side when concurrentFeatures enabled.

Is that possible to let emotion have a separate worker like bundle as described above? If so I think we can try to test it together and push it forward

@Andarist
Copy link
Contributor

We already need this to support Cloudflare Workers so we need to add support for a worker condition anyway. We are sort of blocked on adding the support for such output bundles to be added to the bundling tool that we are using, here is the ticket: preconstruct/preconstruct#431 . Luckily the tool is managed by us as it came out of our old build tooling but TLDR is that adding this to Emotion is not a one-line change.

I wont have time this week to work on this but I hope to start working on this week after that

@huozhi
Copy link
Member

huozhi commented Nov 28, 2021

🙏 No rush, I'll keep posted here if we can make it worker field working on our side first

@MDrooker
Copy link

Wondering where this issue is- Still a blocker for moving to React18 and MUI5 with Next

@huozhi
Copy link
Member

huozhi commented Jan 25, 2022

@MDrooker if you're only trying to upgrade to react 18, you don't have to enable streaming SSR (concurrentFeatures). Could you work with only upgrading react to 18rc?

@Andarist
Copy link
Contributor

As to the status for Emotion - we have a PR open to generate package.json#exports in our build tool: preconstruct/preconstruct#435 , once this lands we should be able to include those in Emotion builds and hopefully that should help with the issue. There are still some unknowns to be figured out though.

kodiakhq bot pushed a commit that referenced this issue Mar 16, 2022
Related to #31678 

For streaming pages,

Nodejs runtime: setting global runtime to `"nodejs"` will work with default module resolution.
Edge runtime: previously next.js will pick up browser field since it's the "similar" asset (unlike nodejs) to edge runtime but browser specific things like DOM api could breaks cause edge runtime is more like worker without dom runtime.

This PR is to revert the main field resolution behavior. And if you have a library targeting multiple runtime with different fields, ideally is to make it more isomorphic and will be easy to use in edge runtime
@huozhi
Copy link
Member

huozhi commented Jul 26, 2022

Now when you opt into react 18 with nodejs runtime, the module resolution be same as before.

@huozhi huozhi closed this as completed Jul 26, 2022
@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue was opened via the bug report template.
Projects
None yet
Development

No branches or pull requests

5 participants