-
-
Notifications
You must be signed in to change notification settings - Fork 370
fix(enhanced): Add fallback parsing for container module exposes #4034
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(enhanced): Add fallback parsing for container module exposes #4034
Conversation
|
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Opened previous PR with |
| let entries: Array< | ||
| [exposeKey: string, { import: string[]; name?: string }] | ||
| > = []; | ||
|
|
||
| try { | ||
| // Prefer parsing exposes from stats first | ||
| entries = JSON.parse(data[3]); | ||
| } catch { | ||
| // If that fails, fallback to the original options | ||
| const exposes = this._options.exposes; | ||
|
|
||
| if (!exposes || typeof exposes !== 'object') { | ||
| return; | ||
| } | ||
|
|
||
| entries = Object.entries(exposes); | ||
| } | ||
|
|
||
| entries.forEach(([prefixedName, file]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm opting to maintain the current JSON parsing logic and wrap it in a try catch block. There are probably good reason why it was implemented this way in the first place, but I lack the context, and I rather be as safe/backwards compatible as possible. If it fails the introduced logic just attempts to recover exposes from another source.
@2heal1 @ScriptedAlchemy As you mentioned in the issue, there are probably better ways of solving this, but again, I lack the context to propose a long-term solution. You mentioned a short-term solution, but since the issue was closed, I figured it wouldn't hurt proposing this patch 😅.
Also note I'm open to contribute and implement this in a different way if I can get some high level guidance (Just a couple of quick notes would be really helpful).
| library: { type: 'commonjs-module' }, | ||
| filename: 'remoteEntry.js', | ||
| exposes: { | ||
| './a test module': './test.js', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does break without the patch. Would this be enough test coverage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this is not expected expose key, will this issue only happen in this scene ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@2heal1 Oh, you're absolutely right. But it does break if the spaces are present on the path, say:
{
exposes: {
'./test': './path with spaces/test.js'
}
}I just wanted to modify the test as little as possible, but I can tweak it to represent an actual real scenario!
| // If that fails, fallback to the original options | ||
| const exposes = this._options.exposes; | ||
|
|
||
| if (!exposes || typeof exposes !== 'object') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my investigations this.options.exposes in ModuleHandler is always parsed into
interface ExposesObject {
[k: string]: ExposesConfig;
}
interface ExposesConfig {
import: ExposesItem | ExposesItems;
name?: string;
}by ContainerManager.containerPluginExposesOptions. But due to how everything is currently typed, TS thinks this could be the broader "unparsed" Exposes shape.
type Exposes = (ExposesItem | ExposesObject)[] | ExposesObject;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, we could maintain this 100% decoupled and idempotently "re-parse" the entries ModuleHandler to ensure the type safety, but I would wait for a review first before increasing the complexity here. cc @ScriptedAlchemy @2heal1
|
Does the issue only happen in legacy version ? I want to know which version , if old enough, maybe no need to compate it . Can you provide a minimal reproduce repo ? So i can get more info . |
|
@2heal1 No, it happens on any version, even the latest. Its probably under the radar (although there is an issue open #2684) because it is very unusual to have spaces in paths in "conventional" codebases 😆. But in our use case of Module Federation, we have no control over what the project folder structure looks like (and in fact we have run into the issue more than once), so this is why I'm proposing this fix! Here's the repro, just https://github.com/vinicius-at-webflow/mf-path-issues You should see [webpack-cli] HookWebpackError: Unterminated string in JSON at position 120
SyntaxError: Unterminated string in JSON at position 120
at JSON.parse (<anonymous>)
at ModuleHandler._handleContainerModulebecause of |
|
I can probably add an integration test using this repo I sent you, to avoid any confusion around what this is fixing, exactly, so I guess a test would make everything more clear. |
|
@2heal1 I added a dedicated test case to validate the fix. If you uncomment you should see the same Unterminated string in JSON at position 31; stack = HookWebpackError: Unterminated string in JSON at position 31
at JSON.parse (<anonymous>)
at ModuleHandler._handleContainerModule as the user reports in the issue, as well as in the minimal repro I shared with you |
|
@2heal1 bump |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback". |
|
@2heal1 @ScriptedAlchemy have you had a chance to take a look at the repo I've prepared with the reproduction of the issue? |
|
Merging to branch under our control to see if theres better solution here to adjust pr. Will open pr again after quick investigation |
…) (#4083) Co-authored-by: Vinicius Rocha <vinicius.rocha@webflow.com>
Integrate latest changes from origin/main: - chore(deps-dev): bump publint from 0.2.12 to 0.3.13 (#4109) - chore(deps): bump @changesets/get-dependents-graph from 2.1.2 to 2.1.3 (#4110) - fix enhanced type tests (#4113) - fix(enhanced): Add fallback parsing for container module exposes (#4034) (#4083) Resolved pnpm-lock.yaml conflict by regenerating the lockfile. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>

Description
The
_handleContainerModulemethod frompackages/manifest/src/ModuleHandler.tscallssplit(' ')on the module'sidentifierwhich breaks if the path or expose key contain spaces or other special characters.Here's a an example of a problematic identifier (see the
importpath for theButtonexpose):Related Issue
#2684
Types of changes
Checklist