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

clientModules are still evaluated server-side instead of client-side #4268

Closed
jhackett1 opened this issue Feb 22, 2021 · 14 comments
Closed

clientModules are still evaluated server-side instead of client-side #4268

jhackett1 opened this issue Feb 22, 2021 · 14 comments
Labels
bug An error in the Docusaurus core causing instability or issues with its execution closed: working as intended This issue is intended behavior, there's no need to take any action.

Comments

@jhackett1
Copy link

jhackett1 commented Feb 22, 2021

Hello,

i'd like to include some third party global javascript code in my docusaurus website.

the third-party code is not ssr-safe - it includes references to window and document objects and i can't change it (eg. to wrap it in an if document then... conditional).

To Reproduce

I've written a tiny js file that imports the js code and initialises it. it looks a bit like this:

import { initAll } from "../lbh/all";

initAll();

and then, in my docusaurus.config.js file, i include that file as a clientModule:

clientModules: [require.resolve("./src/docs.js")],

This runs great in development, but when I do docusaurus build it breaks with ReferenceError: document is not defined.

I assume docusaurus is still trying to include that third-party code in the server bundle somehow.

Possibly this isn't a bug and I've misunderstood what clientModules is supposed to be used for. If so, perhaps we could improve the docs to make this clear?

Expected behavior

Build succeeds.

Actual Behavior

ReferenceError: document is not defined

Your Environment

Reproducible Demo

https://codesandbox.io/s/twilight-snowflake-zocv2

also raised in the discord

@jhackett1 jhackett1 added bug An error in the Docusaurus core causing instability or issues with its execution status: needs triage This issue has not been triaged by maintainers labels Feb 22, 2021
@slorber
Copy link
Collaborator

slorber commented Feb 22, 2021

Yes that seems like a bug to me.

You can probably work around this temporarily by using:

if (typeof window !== "undefined") {
  require("../lbh/all").initAll();
}

We'll figure out how to exclude client modules from the server build.

Note for myself: the client modules seems to be imported on the SSR pipeline with import clientModules from '@generated/client-modules';.

@jhackett1
Copy link
Author

good to know i'm not going mad! your workaround does the trick for now

@Josh-Cena Josh-Cena removed the status: needs triage This issue has not been triaged by maintainers label Oct 30, 2021
@Josh-Cena Josh-Cena changed the title Build broken with "document is not defined" despire using clientModules? clientModules are still evaluated server-side instead of client-side Oct 30, 2021
@JoshuaKGoldberg
Copy link
Contributor

We should note that the original call stack isn't being preserved. If this bug ends up being a wontfix / intentional, it'd be nice to have that fixed.

https://app.netlify.com/sites/typescript-eslint/deploys/6190117990e17f0007564e0d:

2:28:46 PM: [success] [webpackbar] Server: Compiled with some errors in 49.46s
2:28:46 PM: ReferenceError: document is not defined
2:28:46 PM: Unable to build website for locale "en".
2:28:47 PM: Error: Failed to compile with errors.
2:28:47 PM:     at /opt/build/repo/node_modules/@docusaurus/core/lib/webpack/utils.js:202:24
2:28:47 PM:     at /opt/build/repo/node_modules/webpack/lib/MultiCompiler.js:554:14
2:28:47 PM:     at processQueueWorker (/opt/build/repo/node_modules/webpack/lib/MultiCompiler.js:491:6)
2:28:47 PM:     at processTicksAndRejections (node:internal/process/task_queues:78:11)
2:28:47 PM: error Command failed with exit code 1.

@Josh-Cena
Copy link
Collaborator

Fixing this issue is trivial (just do a conditional import of the client modules), the only problem being whether we want to do it or not. I feel like we don't even have a clear definition of what client modules are, what they are supposed to do, and how they should interact with the Docusaurus app itself.

@JoshuaKGoldberg Given that the client modules are directly required as side-effect when mounting the React app, what would you think we could do to provide the stack?

@JoshuaKGoldberg
Copy link
Contributor

This is my first time peeking into the Docusaurus source code so just thinking out loud... Going off of slorber's helpful hint, the App.tsx imports client modules via:

import './client-lifecycles-dispatcher';

import clientModules from '@generated/client-modules';

If you're ok with using top-level async/await, you could:

try {
  await import('./client-lifecycles-dispatcher');
} catch (error) {
  console.error("Oh no! Failed to load client modules!", error);
}

...perhaps wrapped with an if (process.env ... / if (typeof window ... or something like that.

An alternative would be to drop the try/catch and await and use import(...).catch(...). But that would make the timing asynchronous and nondeterministic on the client, which feels to me like a no-go.

But, again, I'm super unfamiliar with the Docusaurus execution model so those are just suggestions 😄

@Josh-Cena
Copy link
Collaborator

Yes, I think you've got the idea about where it's coming from. We just had some troubles with client modules' side effects (#6052) so I did look a bit into this. And when I said "conditional import" I mean:

if (typeof window !== 'undefined') {
  require('./client-lifecycles-dispatcher');
}

Because we use CJS throughout, we don't necessarily need to await import (and we don't even have ESM support yet—#5816 is decided to be postponed to v3).

However that still wouldn't make the error stack visible, because there's no caller here, just a bunch of require calls...

@slorber
Copy link
Collaborator

slorber commented Dec 20, 2021

This is the content of the generated .docusaurus/client-modules.js file:

export default [
  require("/Users/sebastienlorber/Desktop/projects/docusaurus/node_modules/infima/dist/css/default/default.css"),
  require("/Users/sebastienlorber/Desktop/projects/docusaurus/packages/docusaurus-theme-classic/lib/prism-include-languages"),
  require("/Users/sebastienlorber/Desktop/projects/docusaurus/packages/docusaurus-theme-classic/lib/admonitions.css"),
  require("/Users/sebastienlorber/Desktop/projects/docusaurus/website/src/css/custom.css"),
  require("/Users/sebastienlorber/Desktop/projects/docusaurus/website/_dogfooding/clientModuleExample.ts"),
];

This concept of client modules exist for a while, and I'm also not 100% sure what was the initial purpose.

https://docusaurus.io/docs/lifecycle-apis#getclientmodules

Returns an array of paths to the modules that are to be imported in the client bundle. These modules are imported globally before React even renders the initial UI.

In practice, this is used to load global code in the client app. We run the client app and import those modules too in the SSR process. Not sure what kind of weird side effects this would lead to if we only import those modules on the browser app, skipping the SSR app?

Maybe a good solution would be to just change the name to getModules instead of getClientModules?


Not sure how to get a better stacktrace.

This is what Webpack gives us in the stats object:

image

I tried to play with a few options without success. @alexander-akait any idea?

One possibility is that we wrap the generated file requires() in try/catch to at least tell the user which client module is failing to load, but still not perfect.

@alexander-akait
Copy link

What is webpack version? undefined should work, maybe another package inject browser code for Node.js? Also do you have mini-css-extract-plugin and use web workers?

@slorber
Copy link
Collaborator

slorber commented Dec 20, 2021

webpack@5.64.1

undefined should work, maybe another package inject browser code for Node.js? Also do you have mini-css-extract-plugin and use web workers?

I don't understand what you mean. My goal is just to get a stacktrace alongside the error message, not to debug a CSS loading issue.

What I want is Webpack to provide us with an easy to debug error like this:

{
  errors: [
    {
       message: "ReferenceError: document is not defined",
+       error: "ReferenceError: document is not defined
+           at   /Users/sebastienlorber/Desktop/projects/docusaurus/website/_dogfooding/clientModuleExample.js:12
+           at   /Users/sebastienlorber/Desktop/projects/docusaurus/website/docusaurus/client-modules.js:6"
    }
  ]
}

@alexander-akait
Copy link

alexander-akait commented Dec 20, 2021

Sorry, I think you need help to identify the problem, we have bundled code, so we don't have good original stack trace, if we need good stack trace we need source maps (like we do in browser), you can try https://nodejs.org/dist/latest-v14.x/docs/api/cli.html#cli_enable_source_maps, I don't try it, but I think it should work (and you need to set devtool: 'source-map')

@slorber
Copy link
Collaborator

slorber commented Dec 20, 2021

I see thanks 👍 will try that

@Josh-Cena
Copy link
Collaborator

I'll probably be closing this as a wontfix, because over time, I start to understand "client module" here with "client" being "webpack" rather than "client-side rendering", and side-effects a client module induces could be equally useful in SSR, even if it means making people writing typeof window !== 'undefined' in most instances...

I'll leave it open to remind us that there's the source map problem, but this is totally different. All SSR errors have very poor stack traces.

@slorber
Copy link
Collaborator

slorber commented Feb 2, 2022

Yes, it's a bit confusing because we have 3 things:

    1. Node code
    1. Client code that runs during SSR through Webpack server entry
    1. Client code than runs in the browser through Webpack client entry

The question is clientModules should be run in only 3 or 2+3 🤷‍♂️

For now, it's 2 + 3, which is more generic/flexible, but both can make sense eventually, it's hard to tell without a list of use-cases (and I think the initial one was the Google Analytics plugin)

We might has well provide a new option for modules initialized only in 3, and yet keep the current clientModules as is, I don't know if having 2 configs wouldn't be more confusing anyway 😅

@Josh-Cena Josh-Cena added the closed: working as intended This issue is intended behavior, there's no need to take any action. label May 20, 2022
@Josh-Cena
Copy link
Collaborator

Opened #7456. I'll close this as a working-as-intended, now that this behavior is documented. Our client modules API is designed with maximum flexibility in mind because we don't really want many APIs that do similar stuff.

@Josh-Cena Josh-Cena closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution closed: working as intended This issue is intended behavior, there's no need to take any action.
Projects
None yet
Development

No branches or pull requests

5 participants