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

feat(nextjs-mf): enable JSON manifest for NextJS #2726

Merged
merged 11 commits into from
Nov 17, 2024

Conversation

benmarch
Copy link
Contributor

@benmarch benmarch commented Jul 7, 2024

Description

The NextJS plugins are not working with the MF 2.0 manifest because the publicPath is not properly generated on either the server or client. Additionally, adding getPublicPath causes the NextJS build to fail and I haven't been able to track down the root cause yet. These changes enhance the runtime plugin included with the NextJS MF plugin to reassign the publicPath to ensure that requests made after the initial manifest fetch are correctly resolved.

Changing the manifest filePath in NextJS MF plugin results in the manifest file being output in the wrong location, and the manifest itself is generated based on relative paths to the output. So modifying it in the runtimePlugin is the simplest (though maybe not the most robust) approach. These changes don't cause existing remoteEntry.js configures to break.

Since there are no tests for the runtimePlugin, I didn't add any new ones. Happy to write some if someone can let me know where. I created a new example in the module-federation-examples repo to demo this: module-federation/module-federation-examples#4231

Please let me know what you think and if you would prefer a different approach.

Related Issue

#2673

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 Jul 7, 2024

🦋 Changeset detected

Latest commit: 02fdeb3

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

This PR includes changesets to release 1 package
Name Type
@module-federation/nextjs-mf Minor

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 Jul 7, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit 02fdeb3
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/673a49f95d3e100008cd9158
😎 Deploy Preview https://deploy-preview-2726--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.

Copy link
Contributor

Stale pull request message

@ciandt-crodrigues
Copy link
Contributor

@ScriptedAlchemy I was looking forward on having that added to support MF2.0
what does that need to be merged? Do you need any help?

@ScriptedAlchemy
Copy link
Member

ScriptedAlchemy commented Nov 17, 2024

Since support is ending, adding a new feature to the plugin is not something I particularly want to do, as any problems it may contain will open more issues on the repository that I will not support or fix.

JSON protocol means HMR, Chrome tools, and TypeScript will work. If there are any issues with Next, users will open issues and create noise.

Without a lead maintainer, it will most likely create more problems than it's worth. Since Federation barely works already with Next, this would probably open the floodgates versus locking users out of the majority of features for their own good.

Contacting vercel is the best approach

@ciandt-crodrigues
Copy link
Contributor

Since support is ending, adding a new feature to the plugin is not something I particularly want to do, as any problems it may contain will open more issues on the repository that I will not support or fix.

JSON protocol means HMR, Chrome tools, and TypeScript will work. If there are any issues with Next, users will open issues and create noise.

Without a lead maintainer, it will most likely create more problems than it's worth. Since Federation barely works already with Next, this would probably open the floodgates versus locking users out of the majority of features for their own good.

Contacting vercel is the best approach
So "Community pull requests will continue to be merged." from the deprecation notice was not accurate, Maybe let it clear that it will be only bugfixes?

When I started building Module Federation was an webpack5 feature, so I had the impression it would be maintained by them.

We started the app using regular React + Webpack + Module federation with almost 100 exposed components.
But due to Performance and SEO we needed to migrate to SSR. NextJS still is the most used framework for SSR, and with all its flaws it has the most support and knowledge base out there, besides a big company behind. (you must understand how important this is for large enterprise).
We end up going with NextJS because of that and because NextJS/Pages required minimal effort to migrate. (we can keep and use most components on Server and Client pretty easy, and just add some protections on BrowserOnly APIs)
We did the migration, (which was huge, even with the "minimal" adaptation needed) and one week after we get the deprecation notice...

I understand dropping support, you are not obligated to maintain anything. but having the newer versions of the lib just not working and seeing those PR's that could have been merged and fixed by the community not going forward is sad...
I'm now consider if I really need Module Federation (nothing personal), It is maintained for a relative small team, and we have no guarantee that if we change framework the support will not be dropped again. We definitely want to keep the MicroFrontend support but Module Federation is just a flavor for it, not the only option.

You can disconsider my rant, just venting a little. Maybe just update the deprecation notice if possible.

@ScriptedAlchemy
Copy link
Member

ScriptedAlchemy commented Nov 17, 2024

You can rebase the PR if you really want it. I can merge it.
It's not really my fault that vercel made next too painful support from the outside. Lesson learned that In The future we will only support frameworks who want it to exist on their platforms. Most just work with the plugin already, but I was burned on next and won't just make a framework specific plugin unless the framework authors are willing to work with us.

Only the next plugin is maintained by a small team (me). The rest of the ecosystem has a pretty large pool, and v2 is backed by Bytedance, a 400 billion dollar tech company.

If you don't think you need federation, then you probably don't. That said, I've used every other flavor - federation remains the industry standard for a reason.

Understand the venting, especially on next.js, how you feel now has been 4 years of my life lol. All good

On a side note. Vercel apparently is now actively testing Federation v2 as a first class supported tech in next. Nothing actionable as of yet, but it looks encouraging

@ScriptedAlchemy ScriptedAlchemy merged commit 5ad75fd into module-federation:main Nov 17, 2024
15 checks passed
@ciandt-crodrigues
Copy link
Contributor

@ScriptedAlchemy

Thanks for your support man, I really appreciate!

Unfortunately, that was not enough to make nextjs work with manifest file.

when implementing modernjs SSR you guys added an empty default snapshot information and since nextjs manifest does not contain this "new" ssrRemoteEntry it fails.
As a quick workaround I've initialized the default info with the regular remoteEntry and it worked (all my remotes and its modules were just fine).

I've tried to open a PR but I'm having trouble make the unit tests run locally (it seems like clean clone + the guide on the Contribute page is not enough)

Maybe you could help me out?

Also, I think a better permanent solution would be to have the manifest to be generated with the new format (maybe a single one for both ssr and non-ssr)
I could try to work on that if you guide me to the right place to doit once I have more time.

@ScriptedAlchemy
Copy link
Member

test can be run by looking at the .github workflows for E2E or test cases in build-and-test, otherwise just push to CI and let CI do the test runs. Combining SSR and non SSR isnt easy, since there can be many builds - for example RSC will be 6 independent builds - best approach is 2 manifest and have server/client pick and choose the corresponding manifest for its environment since all the data is different as well etc.

You could use a runtimePlugin and rewrite the snapshot on the fly as needed?

@ciandt-crodrigues
Copy link
Contributor

ciandt-crodrigues commented Nov 18, 2024

Thanks @ScriptedAlchemy
the PR can be found at
#3249

@benmarch
Copy link
Contributor Author

Sorry just got caught up. Yes, this change is a bit outdated, and I have an additional change in my runtime plugin to set the ssrRemoteEntry values at runtime like this:

loadRemoteSnapshot(args) {
    // ... all the changes from this PR...

    if (!options.inBrowser) {
      remoteSnapshot.ssrRemoteEntry = remoteSnapshot.remoteEntry
      remoteSnapshot.ssrRemoteEntryType = remoteSnapshot.remoteEntryType
    }

    return args
}

I don't think there is any downside to setting it on the client side, but it won't point to the correct URL (not that it will be used on the client anyway). The way you set it up in your PR @ciandt-crodrigues is much more robust though, thank you!

@ScriptedAlchemy thank you for all the time and effort you put into this plugin. It's definitely a disappointment that support is being dropped, but it's understandable considering the lack of collaboration from Vercel. I'm not sure I have the time to volunteer to maintain the plugin right now, but my company is still using it for a large project so I'll keep working on it as things come up.

@KyrieLii KyrieLii mentioned this pull request Nov 19, 2024
@h3adache
Copy link
Contributor

h3adache commented Dec 6, 2024

This change broke loading of non nextjs remotes. Fix is #3323

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.

4 participants