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: use runtime globals in shared plugin #12831

Merged
merged 4 commits into from
Mar 11, 2021

Conversation

animecyc
Copy link
Contributor

@animecyc animecyc commented Mar 8, 2021

What kind of change does this PR introduce?

Did you add tests for your changes?

I didn't, it would be great if some direction could be given as to where a test for something like this would go. Assuming hotCases?

Does this PR introduce a breaking change?

It doesn't introduce breaking changes.

What needs to be documented once your changes are merged?

This is an internals change, I don't believe this warrants a documentation change.

- Fixes a bug that caused HMR to fail when using Module Federation (__webpack_modules__ is not defined).
@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)

Copy link
Member

@sokra sokra left a comment

Choose a reason for hiding this comment

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

Changes look good, but we definitely need a test case for the bug

@sokra
Copy link
Member

sokra commented Mar 8, 2021

and also make sure to sign the CLA

@sokra
Copy link
Member

sokra commented Mar 8, 2021

Yep, a test case in test/hotCases should make it. To trigger the behavior you can add a shared module in a HMR update. Change an existing module to reference the a new shared module.

To trigger both code paths you need on shared module that it loaded with import ... from "shared1" and another one that is loaded with import("shared2").

@animecyc
Copy link
Contributor Author

animecyc commented Mar 9, 2021

and also make sure to sign the CLA

Will do.

Yep, a test case in test/hotCases should make it. To trigger the behavior you can add a shared module in a HMR update. Change an existing module to reference the a new shared module.

To trigger both code paths you need on shared module that it loaded with import ... from "shared1" and another one that is loaded with import("shared2").

I should be able to get something up here in the next few days. Appreciate the direction.

@webpack-bot
Copy link
Contributor

@animecyc Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@webpack-bot
Copy link
Contributor

@sokra The most important CI builds failed. This way your PR can't be merged.

Please take a look at the CI results from azure (1 errors / 3 warnings) and appveyor (error) and fix these issues.

@sokra sokra force-pushed the bugfix/shared-plugin-runtime-bug branch from 4cec47d to cf57a14 Compare March 11, 2021 17:35
This was referenced Mar 14, 2021
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.

ConsumeSharedPlugin with HMR throws unexpectedly
3 participants