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: Allow hotUpdateGlobal to be a function #18672

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MarioCadenas
Copy link

@MarioCadenas MarioCadenas commented Aug 13, 2024

What kind of change does this PR introduce?

As discussed in this issue this PR allows passing a function to hotUpdateGlobal with the following shape hotUpdateGlobal: (chunk: String) => string, so if for any reason there needs to be multiple runtimes in the same page, and the single runtime is not an option, the hotUpdateGlobal function can be defined by chunk and multiple chunks can have HMR.

Did you add tests for your changes?

I added a test that checks the defaults and integration tests in the hotCases

Does this PR introduce a breaking change?

What needs to be documented once your changes are merged?

Once this is merged and released, this https://webpack.js.org/configuration/output/#outputhotupdateglobal should be updated to show that it can also accept a function, and the shape of it.

I'd be happy to also update the documentation

Copy link

linux-foundation-easycla bot commented Aug 13, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@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

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

please add your case in test/hotCases, we need at least one integration test

@MarioCadenas
Copy link
Author

please add your case in test/hotCases, we need at least one integration test

thanks, that's what I was missing, adding it!

@MarioCadenas
Copy link
Author

MarioCadenas commented Aug 14, 2024

Hey @alexander-akait, I've been trying to add an integration test case, but I can't seem to make multiple entries work. I've been checking other tests to try and understand a bit more how everything works on these test cases, but I couldn't make it work.

Maybe you can point me to some documentation or give me a little bit of guidance?

Pretty much the idea would be to have 2 entries, and somehow "load" them in the same place and try applying some updates to both of them. (That's at least what I've tried in the index.js test file but failed 😞)

I added some comments here https://github.com/webpack/webpack/pull/18672/files#diff-85de71d3c7829fd59bca8bb510eee5872830638ed236bb35fbc3151e1b95374aR6

Any help would be appreciated, thanks a lot!

@MarioCadenas
Copy link
Author

Hey @alexander-akait, I've been trying to add an integration test case, but I can't seem to make multiple entries work. I've been checking other tests to try and understand a bit more how everything works on these test cases, but I couldn't make it work.

Maybe you can point me to some documentation or give me a little bit of guidance?

Pretty much the idea would be to have 2 entries, and somehow "load" them in the same place and try applying some updates to both of them. (That's at least what I've tried in the index.js test file but failed 😞)

I added some comments here https://github.com/webpack/webpack/pull/18672/files#diff-85de71d3c7829fd59bca8bb510eee5872830638ed236bb35fbc3151e1b95374aR6

Any help would be appreciated, thanks a lot!

Never mind, finally figured it out 😄

@MarioCadenas
Copy link
Author

seems like the result is correct

image

@MarioCadenas
Copy link
Author

hey @alexander-akait anything else I should do so this can be reviewed? thank you!

@MarioCadenas MarioCadenas force-pushed the allow-hotupdateglobal-fn branch 2 times, most recently from 50733bb to 5f19810 Compare September 27, 2024 07:55
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Let's do small improve

@MarioCadenas
Copy link
Author

Let's do small improve

Changes ready, let me know if I understood you properly and you were referring to those changes, thanks!

@alexander-akait
Copy link
Member

please run yarn special-lint-fix

@MarioCadenas
Copy link
Author

yarn special-lint-fix

Hey @alexander-akait , already did, It doesn't produce any changes

image

@MarioCadenas
Copy link
Author

hey @alexander-akait it should be ready now 😄

@MarioCadenas
Copy link
Author

hey @alexander-akait could I get a review? thank you!

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