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

Pass shareScope through to ContainerPlugin & ContainerReferencePlugin #16031

Merged
merged 1 commit into from Jul 25, 2022

Conversation

evantd
Copy link
Contributor

@evantd evantd commented Jul 9, 2022

It looks to me like setting shareScope for ModuleFederationPlugin doesn't work without this change, because ContainerPlugin still tries to use default.

What kind of change does this PR introduce?

bugfix

Did you add tests for your changes?

No, but I didn't see any other tests for ModuleFederationPlugin either.

Does this PR introduce a breaking change?

I don't think so.

What needs to be documented once your changes are merged?

I believe this change makes it more closely match existing documentation.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 9, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: evantd / name: Evan Dower (1492735)

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@webpack-bot
Copy link
Contributor

Hi @evantd.

Just a little hint from a friendly bot about the best practice when submitting pull requests:

Don't submit pull request from your own main branch. It's recommended to create a feature branch for the PR.

You don't have to change it for this PR, just make sure to follow this hint the next time you submit a PR.

@evantd
Copy link
Contributor Author

evantd commented Jul 12, 2022

It looks like failures are sort of common. I saw a pull request that only changed the readme, and it also had 2 failed checks. I don't think that these failures are related to my change. It doesn't look like I can retry them either, at least as far as I can tell.

@evantd
Copy link
Contributor Author

evantd commented Jul 12, 2022

It looks like @sokra and/or @ScriptedAlchemy would be the right people to review this.

@ScriptedAlchemy
Copy link
Member

Good catch. Assuming both plugins under the hood still call apply(compiler) - which they should. Then I think this change looks good to me and would ensure plugins still work without having to manually apply 3-4 plugins just to use container or container reference independently

@evantd
Copy link
Contributor Author

evantd commented Jul 12, 2022

Yay! Yeah, I'm using an alternate shareScope for a work project and this change makes it work for me. I currently just apply a patch to node_modules in a postinstall script, and I'd love to stop doing that.

@evantd
Copy link
Contributor Author

evantd commented Jul 12, 2022

Is there anything more that I should be doing to keep this moving forward?

@ScriptedAlchemy
Copy link
Member

@sokra @vankop

@evantd
Copy link
Contributor Author

evantd commented Jul 19, 2022

Since this is approved, should I expect it to be included in the next release?

@ScriptedAlchemy
Copy link
Member

Can take a little time to move through the webpack org depending on what the change is. This seems small so I imagine it would be a quicker turnaround.

Nice thing about webpack is you can insall it over npm off a git branch since there's no build - so that might be useful in the interm

@sokra sokra merged commit 6dc6a19 into webpack:main Jul 25, 2022
@sokra
Copy link
Member

sokra commented Jul 25, 2022

Thanks

@evantd
Copy link
Contributor Author

evantd commented Oct 20, 2022

I found myself needing to check if my version was new enough, so for anybody else (or future me), this is fixed in 5.74.0.

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.

None yet

5 participants