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

fix(runtime) Fix snapshot for server manifest without ssr remote entry #3249

Conversation

ciandt-crodrigues
Copy link
Contributor

Description

When trying to use MF2.0 manifest as entrypoint for the nextjs-mf I found an issue where the snapshot was being rejected. the cause is that the manifest for nextjs-mf does not include a ssrRemoteEntry property (introduced a few months ago for modernJS).
This PR addresses that issue falling back to the regular remoteEntry for server-side manifests.

Related Issue

Snapshot fails for server manifest without ssr remote entry
It also completes the work started here

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

Copy link

changeset-bot bot commented Nov 18, 2024

🦋 Changeset detected

Latest commit: 734d305

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 27 packages
Name Type
@module-federation/runtime Major
@module-federation/devtools Major
@module-federation/data-prefetch Major
@module-federation/dts-plugin Major
@module-federation/nextjs-mf Patch
@module-federation/node Patch
@module-federation/retry-plugin Major
@module-federation/runtime-tools Major
@module-federation/webpack-bundler-runtime Major
@module-federation/bridge-react Major
@module-federation/bridge-vue3 Major
@module-federation/enhanced Major
@module-federation/modern-js Major
@module-federation/rspack Major
@module-federation/rsbuild-plugin Major
@module-federation/storybook-addon Patch
@module-federation/modernjsapp Patch
@module-federation/sdk Major
@module-federation/managers Major
@module-federation/manifest Major
@module-federation/third-party-dts-extractor Major
@module-federation/bridge-shared Major
@module-federation/bridge-react-webpack-plugin Major
@module-federation/error-codes Major
@module-federation/esbuild Patch
@module-federation/utilities Patch
website-new Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

netlify bot commented Nov 18, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit 734d305
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/673b334e8baa4b0008e6ebb4
😎 Deploy Preview https://deploy-preview-3249--module-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@2heal1
Copy link
Member

2heal1 commented Nov 20, 2024

@ScriptedAlchemy Is this(next-js manifest) remoteEntry build target node? I'm worried about the remoteEntry build target is browser which can not work on node env .

@ScriptedAlchemy
Copy link
Member

Both builds produce independent manifest. 1 for browser and 1 for server. But only modernjs makes the ssr remote entry key even tho it's a cjs based build - manifest emits remoteEntry key like any standalone build

@ScriptedAlchemy
Copy link
Member

@ciandt-crodrigues Instead of changing our runtime for next, i think you should look at making a new plugin or adding another hook to the next plugin on process assets, find the manifest and object assign/rewrite the keys as needed in the build plugin - just for nextjs-mf without altering the runtime itself - just mutate the asset if isServer == true

@ciandt-crodrigues
Copy link
Contributor Author

@ScriptedAlchemy I can definitely do that, and it will work for me, but not everyone.
I still feel like this is a bug where modernjs requirements leaked to the runtime (which I though was supposed to be agnostic, right?) ( the ssrRemoteEntry on manifest is a modernJs concept only IMO, not something that was changed for ModuleFederation 2.0) .

I feel like this fix addresses that. while keeping both environments functional.

Just let me know if you guys have interest on keeping that or not.
If not I can do that on my project only, but I feel like this is something cool to share with everyone on the community.

@ScriptedAlchemy
Copy link
Member

You can send pr to nextjs-mf to merge manifest etc similarly to how modernjs does it.
This is how our systems work at bytedance, assuming remoteEntry may be a server remote would make it too easy to execute code in the wrong environment, servers dont need a js manifest.

In the future we rspack will support multi-target outputs from a single compile, so its a short term issue imo.

Feel free to add this on the plugin end of nextjs-mf so all users can use it. server info should be explicitly separate from browser info otherwise there is no way to detect the intent and it will be very easy to take production down.

Because builds are not homogenous, there is no automatic way to merge manifests between the two, since we do not know where each build is emitting assets outside of how a framework does it - like next.js, we know where it goes so one could merge them, same with modernjs, if a rando wants to implement it - its not a magic feature we can enable between two build phases.

Our SSR implementation in modernjs is also subject to change still, probably next quarter we will churn through another iteration and could look at this again, but either there would need to be a explicit difference in key reference for server vs client entry imo.

Can likely be smoothed out in the future but will require more thought and consideration around this aspect would be needed. But def send a PR to next mf with a processasset hoook and update the asset if compiler.name === 'server'

You could also find a place in our docs or make a new section somewhere to put down some tips etc around node/server + ssr based module loading with the runtime etc if you wanted to so anyone who does want to handroll had some guidance on what to do to correlate between the two builds

@ciandt-crodrigues
Copy link
Contributor Author

@ScriptedAlchemy
Thanks! I'll do that when I have more time, I'm closing this PR then.
So the idea is just to do that on the loadRemoteSnapshot of the runtimePlugin on nextjf-mf, right?

@ScriptedAlchemy
Copy link
Member

You can do it on runtime plugin. Or you could do it by adding another compiler hook like compilation.hooks.processAssets.tapAsync and actually assign the json to be ssrRemote for the compiler named server.

Either will work, but if you want the json to output with right name key, you can catch the asset in webpack pipeline and adjust it then return the updated asset source which is emitted

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

Successfully merging this pull request may close these issues.

3 participants