Skip to content

fix: #156 ssr: require is not defined #250

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

poenneby
Copy link

@poenneby poenneby commented Feb 6, 2025

Further to this discussion: #156 (comment)

Copy link
Collaborator

@gioboa gioboa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @poenneby for your help 🤗

@poenneby
Copy link
Author

poenneby commented Feb 7, 2025

@gioboa Running the multi-example with these changes correctly export default exportModule but at runtime I get "unexpected token: default"
Tracing this back in the code the export default gets incorrectly identified as a named export in src/plugins/pluginDevProxyModuleTopLevelAwait.ts

The transpiled code that causes the error:

// node_modules/__mf__virtual/host__loadRemote__remote_mf_1_Product__loadRemote__.js
var import_node_module = __toESM(require_node_module());
var { loadRemote } = require2("@module-federation/runtime");
var { initPromise } = require2("__mf__virtual/host__mf_v__runtimeInit__mf_v__.js");
var res = initPromise.then((_) => loadRemote("remote/Product"));
var exportModule = (
  /*mf top-level-await placeholder replacement mf*/
  initPromise.then((_) => res)
);
var require2 = (0, import_node_module.createRequire)(import.meta.url);
var host_loadRemote_remote_mf_1_Product_loadRemote_default = exportModule;

              const __mfproxy__awaitdefault = await default(); <-- HERE
              const __mfproxy__default = () => __mfproxy__awaitdefault;
            
export { __mfproxy__default as default };

Any ideas?

@gioboa
Copy link
Collaborator

gioboa commented Feb 7, 2025

I see, I will check it. Thanks for the deep investigation

@rafal-akiro
Copy link

Hey @gioboa any update on this?

@gioboa
Copy link
Collaborator

gioboa commented Feb 10, 2025

I did look at it yet. But eventually we can continually change the code if an SSR env is detected

@poenneby
Copy link
Author

poenneby commented Mar 5, 2025

Hey @gioboa ! Are you looking at this. Would be happy to help but I am struggling to understand the inner workings. Thanks!

@gioboa
Copy link
Collaborator

gioboa commented Mar 5, 2025

Not yet but I will look at it. Can you share your repository to test this change please?

@poenneby
Copy link
Author

poenneby commented Mar 7, 2025

Not yet but I will look at it. Can you share your repository to test this change please?

Here you go! https://github.com/poenneby/mf-vanilla-react-router

@tiagobnobrega
Copy link

tiagobnobrega commented Mar 12, 2025

Hi @poenneby. I'm trying to do the same as you. I fond this issue and started some investigation. I don't think you can rely on checking the remote type for esm. This would work if the exported code would only run on SSR. But that module can be requested by the browser directly, as in the failing test.

So to make it compatible with client-side rendering it needs to use syntax supported in CJS. Even if we fix the top-level await issue, the import from 'node:module' wouldn't work in this scenario.

I guess the ideal solution would be use a syntax supported both by CJS and ESM (which is easier said than done). I think we can try using dynamic imports and runtime check on the exports. something like this:

export function generateRemotes(id: string, command: string) {
  return `
    const runtimePromise = import('@module-federation/runtime');
    const virtualRuntimePromise = import("${virtualRuntimeInitStatus.getImportId()}");
    const res = Promise.all([runtimePromise, virtualRuntimePromise])
      .then(([runtime, virtualRuntime]) => {
        const initPromise = virtualRuntime.initPromise;
        return initPromise.then(() => runtime.loadRemote(${JSON.stringify(id)}));
      });
    const exportModule = ${command !== 'build' ? '/*mf top-level-await placeholder replacement mf*/' : 'await '}res;
    
    // Dual exports for CJS/ESM compatibility
    if (typeof module !== 'undefined') {
      module.exports = exportModule;
      module.exports.default = exportModule;
    }
    
    // ESM export fallback
    if (typeof exports !== 'undefined' && typeof define !== 'function') {
      Object.defineProperty(exports, '__esModule', { value: true });
      exports.default = exportModule;
    }
  `;
}

This is passing the current tests.
Ideally we would need to also add a SSR test to validate this on a node/deno/bun environments. Haven't tested in SSR yet.
Also, I don't love this solution, it's kind of a mess. We could try UMD, but that's just a different flavor of mess.

Other than that I think we would need to figure out a way to properly identify the runtime (CJS/ESM) instead of the relying on the remote type.

EDIT: This also seems to break the toplevel await plugin logic.

@tiagobnobrega
Copy link

Another idea is to use a plugin (https://github.com/originjs/vite-plugins/tree/main/packages/vite-plugin-commonjs) to convert CJS to ESM on the host doing SSR and requesting the module.
I'll try out a few more things today, but I'm not sure how long I can spend on this ATM.

@tiagobnobrega
Copy link

ok... this is a slightly modified code, that doesn't seem to affect top level await logic:

export function generateRemotes(id: string, command: string) {
  return `
    const runtimePromise = import('@module-federation/runtime');
    const virtualRuntimePromise = import("${virtualRuntimeInitStatus.getImportId()}");
    const res = Promise.all([runtimePromise, virtualRuntimePromise])
      .then(([runtime, virtualRuntime]) => {
        const initPromise = virtualRuntime.initPromise;
        return initPromise.then(() => runtime.loadRemote(${JSON.stringify(id)}));
      });
    const exportModule = ${command !== 'build' ? '/*mf top-level-await placeholder replacement mf*/' : 'await '}res;

    // Dual exports for CJS/ESM compatibility
      module.exports = exportModule;
      module.exports.default = exportModule;
    // ESM export fallback
      const _exports = exports || {};
      Object.defineProperty(_exports, '__esModule', { value: true });
      exports.default = exportModule;
  `;
}

but the weird thing is that now the build fails, from what it seems like we want the top level await transform to do.

error during build:
[commonjs--resolver] node_modules/__mf__virtual/host__loadRemote__remote_mf_1_Product__loadRemote__.js (9:31): await isn't allowed in non-async function
file: /Users/tiago/ws/other/vite/examples/vite-webpack-rspack/host/node_modules/__mf__virtual/host__loadRemote__remote_mf_1_Product__loadRemote__.js:9:31

 7:         return initPromise.then(() => runtime.loadRemote("remote/Product"));
 8:       });
 9:     const exportModule = await res;
                                   ^
10:
11:     // Dual exports for CJS/ESM compatibility

RollupError: await isn't allowed in non-async function

Not sure what's happening and why we need the top level await logic exactly.

@tiagobnobrega
Copy link

@poenneby I think I have a slightly better understanding of it now. We were making it harder than it had to be. We don't have to make the exports esm compatible. Rollup will process the file before writing it, and wrap the code into a __commonJS call. But we can't use the require calls as those are not available on esm mode.

So, I think we only need to get rid of the require calls.

I've got a fork for this here :https://github.com/tiagobnobrega/mf-vite/tree/interop
It's on the interop branch.

Haven't tested on SSR yet. I'll try to make progress on that if I find the time.

@zd754
Copy link

zd754 commented Apr 7, 2025

Is there any update on this?

@pcfreak30
Copy link

@poenneby I think I have a slightly better understanding of it now. We were making it harder than it had to be. We don't have to make the exports esm compatible. Rollup will process the file before writing it, and wrap the code into a __commonJS call. But we can't use the require calls as those are not available on esm mode.

So, I think we only need to get rid of the require calls.

I've got a fork for this here :https://github.com/tiagobnobrega/mf-vite/tree/interop It's on the interop branch.

Haven't tested on SSR yet. I'll try to make progress on that if I find the time.

im running into a related issue that may require the use of esm import() vs require. It seems that there is an edge case that can cause an import cycle, and as cjs gets rewritten to esm, the page hangs b/c a module waits on a promise that loads a chunk that goes and ends up requesting itself...

So far rewriting writeLoadShareModule may be needed.

@tiagobnobrega
Copy link

Sorry. I had to move focus elsewhere. I'm still interested in this, but it is more complex than I anticipated. I found issues with top-level await in ESM modules. I was looking at the runtime packages, maybe this could leverage that ecosystem more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants