Allow HMR status handlers to return a Promise #13576

rockwalrus · 2021-06-17T00:17:42Z

This change allows handlers registered using addStatusHandler to return a Promise. The HMR system will wait until the promise settles before continuing. This allows asynchronous handling of state changes.

In our case, we use this to make sure that when our service worker is modified the update is complete and the service worker installed before the dev server triggers a reload on an unaccepted update.

The diff is much easier to read with ignore whitespace turned on.

What kind of change does this PR introduce?
New feature.

Did you add tests for your changes?
Yes.

Does this PR introduce a breaking change?
No. Handler return values were ignored before, but the worst thing that could happen in the unlikely case that someone was inadvertently returning a Promise from a handler that they weren't expecting to be waited for is that the HMR process might be delayed.

What needs to be documented once your changes are merged?
Document in the Hot Module Replacement Management API section that if a status handler returns a Promise that it will wait for the Promise before continuing.

The HMR system will wait until the promise settles before continuing.

webpack-bot · 2021-06-17T00:17:47Z

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)

sokra · 2021-06-18T07:38:06Z

lib/hmr/HotModuleReplacement.runtime.js

-					for (var i = 0; i < modules.length; i++) {
-						outdatedModules.push(modules[i]);
+			// Now in "apply" phase
+			return setStatus("apply").then(function () {


Here is the only problem I'm seeing.

By this being a Promise, dispose and apply does no longer run in a single go.
This introduces the ability that user code is run in between, while the runtime is in a incomplete state.

e. g. A timeout could fire which imports a module, which is correctly in process of being replaced.

We can't introduce a break here, but I think it would work to all setState("apply") and await the Promise later (before setStatus("idle")/setStatus("fail")).

That will complicate documentation somewhat, but I agree that it's the best option. I considered just not supporting Promises at all for dispose and apply, but it seemed uglier to support Promises for some statuses but not others, and this does provide the somewhat useful ability to write code that always runs after apply is finished regardless of whether there is an error or not.

You can explain it as "running in parallel to the normal apply logic".

sokra · 2021-06-18T07:42:45Z

test/hotCases/invalidate/during-idle/index.js

-	expect(a.value).not.toBe(oldA);
-	expect(b.value).not.toBe(oldB);
-	expect(c.value).toBe(oldC);
+	module.hot.apply().then(function () {


Make sure that the apply() API stay sync as long there is no async status handler registered, to avoid a breaking change.

Since setStatus is only internal you could return a { then: fn => fn() } when no Promise was returned from status handlers

Depending on the synchronously-executed part of an API that's documented to return a Promise seems dangerous, and even before this patch error handling is broken if you try to do so. But I can still implement the synchronous then() if you'd like.

webpack-bot · 2021-06-22T00:23:34Z

@rockwalrus Thanks for your update.

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

@sokra Please review the new changes.

sokra · 2021-06-22T11:36:37Z

Could you sign the CLA: https://cla.js.foundation/webpack/webpack?pullRequest=13576 ?

rockwalrus · 2021-06-22T16:33:34Z

Could you sign the CLA: https://cla.js.foundation/webpack/webpack?pullRequest=13576 ?

Do you have a corporate CLA that State Farm can sign on my behalf?

Thanks.

alexander-akait · 2021-06-22T16:41:13Z

Do you have a corporate CLA that State Farm can sign on my behalf?

We have only general CLA...

rexb1971 · 2021-06-24T16:13:43Z

I see that the CLA is still showing as being unsigned, but an officer of our company has now signed the document and affirmed that State Farm owns copyright of the contributing code, so I am hoping that you can validate that his signature has been accepted and move the status from unsigned to signed.

sokra · 2021-06-28T08:03:17Z

@rockwalrus you have to click this link and sign there: https://cla.js.foundation/webpack/webpack?pullRequest=13576
After that the bot will flag the CLA as accepted (after closing and reopening, because it's a bit buggy)

rockwalrus · 2021-06-29T19:07:18Z

I don't have authorization to sign on behalf of the copyright owner, @StateFarmIns. The executive in charge of open source contribution, @RandyMcBeath, who does have that authorization, signed the CLA. Is there a way for you to manually mark that the CLA has been signed for this request? If not, State Farm is going to have to officially license the code to me, at which point I'll be legally able to license the code to you by signing the CLA. But that will take some time to accomplish.

sokra · 2021-06-30T12:22:01Z

I don't have authorization to sign on behalf of the copyright owner, @StateFarmIns.

Yeah, but that's not what you need to do. You only need to sign it for yourself.

By signing the CLA you only certify that you created that code, and you are allowed to submit it under our Licence (MIT). Since your company seem to allow it, that should be fine.

But I'm not a lawyer so not super sure...

Is there a way for you to manually mark that the CLA has been signed for this request?

Not really. We could merge the PR without CLA, but that's not the idea of that...

Actually nobody requested that in the last > 5 year since he have this CLA...

rexb1971 · 2021-06-30T13:39:01Z

I don't have authorization to sign on behalf of the copyright owner, @StateFarmIns.

Yeah, but that's not what you need to do. You only need to sign it for yourself.

By signing the CLA you only certify that you created that code, and you are allowed to submit it under our Licence (MIT). Since your company seem to allow it, that should be fine.

But I'm not a lawyer so not super sure...

Is there a way for you to manually mark that the CLA has been signed for this request?

Not really. We could merge the PR without CLA, but that's not the idea of that...

Actually nobody requested that in the last > 5 year since he have this CLA...

@sokra - according to our lawyers, the problem is that the copyright for this code is no longer held by @rockwalrus - it is instead held by State Farm. So @rockwalrus no longer has the "right" to affirm the terms of the CLA unless we somehow transfer the copyright back to him. So we will need to do our own legal paperwork in order for him to legally sign this CLA himself - according to our lawyers.

rockwalrus · 2021-06-30T17:42:45Z

Sorry for the delay. We'll let you know when we've worked this out internally.

rockwalrus · 2021-07-16T17:26:45Z

I am now authorized to sign the CLA, and have done so!

alexander-akait · 2021-07-16T17:32:32Z

we rewrite tests, maybe make sense to do rebase for CI

webpack-bot · 2021-07-17T05:12:04Z

@rockwalrus 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 / 1 warnings) and appveyor (success) and fix these issues.

sokra · 2021-07-18T17:02:27Z

Thanks

rockwalrus · 2021-07-19T16:21:46Z

You're welcome!

webpack-bot · 2021-07-22T10:06:19Z

I've created an issue to document this in webpack/webpack.js.org.

Allow HMR status handlers to return a Promise

e852415

The HMR system will wait until the promise settles before continuing.

webpack-bot added the PR: unreviewed label Jun 17, 2021

webpack-bot added PR: CI-ok PR: CI-not-ok and removed PR: CI-ok labels Jun 17, 2021

sokra requested changes Jun 18, 2021

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: unreviewed labels Jun 18, 2021

Dispose and apply must run atomically.

ef4aa56

webpack-bot removed the PR: reviewed-changes-requested label Jun 22, 2021

webpack-bot added PR: review-outdated PR: CI-ok and removed PR: CI-not-ok labels Jun 22, 2021

sokra approved these changes Jun 22, 2021

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Jun 22, 2021

alexander-akait closed this Jun 24, 2021

alexander-akait reopened this Jun 24, 2021

sokra closed this Jun 28, 2021

sokra reopened this Jun 28, 2021

webpack-bot added PR: CI-not-ok and removed PR: CI-ok labels Jun 29, 2021

chenxsan mentioned this pull request Jul 6, 2021

use correct URL encoded value for space webpack/webpack.js.org#5155

Closed

alexander-akait closed this Jul 16, 2021

alexander-akait reopened this Jul 16, 2021

sokra merged commit a87dba4 into webpack:main Jul 18, 2021

webpack-bot mentioned this pull request Jul 22, 2021

Document webpack change: Allow HMR status handlers to return a Promise webpack/webpack.js.org#5213

Closed

stefan-toubia mentioned this pull request Jul 24, 2021

HMR Infinite request loop after restarting webpack serve #13857

Closed

cskeogh mentioned this pull request Jul 26, 2021

Webpack 5.45.0 error "Module parse failed: Unexpected token (2:7)" Webpack 5.46.0 repeats "[HMR] Nothing hot updated." jaredpalmer/razzle#1666

Closed

System233 mentioned this pull request Aug 2, 2023

fix: The setStatus() of the HMR module should not return an array, which may cause infinite recursion. #17556

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow HMR status handlers to return a Promise #13576

Allow HMR status handlers to return a Promise #13576

rockwalrus commented Jun 17, 2021

webpack-bot commented Jun 17, 2021 •

edited by sokra

sokra Jun 18, 2021

rockwalrus Jun 18, 2021

sokra Jun 21, 2021

sokra Jun 18, 2021

rockwalrus Jun 22, 2021

webpack-bot commented Jun 22, 2021

sokra commented Jun 22, 2021

rockwalrus commented Jun 22, 2021

alexander-akait commented Jun 22, 2021

rexb1971 commented Jun 24, 2021

sokra commented Jun 28, 2021

rockwalrus commented Jun 29, 2021

sokra commented Jun 30, 2021

rexb1971 commented Jun 30, 2021

rockwalrus commented Jun 30, 2021

rockwalrus commented Jul 16, 2021

alexander-akait commented Jul 16, 2021

webpack-bot commented Jul 17, 2021

sokra commented Jul 18, 2021

rockwalrus commented Jul 19, 2021

webpack-bot commented Jul 22, 2021

Allow HMR status handlers to return a Promise #13576

Allow HMR status handlers to return a Promise #13576

Conversation

rockwalrus commented Jun 17, 2021

webpack-bot commented Jun 17, 2021 • edited by sokra

sokra Jun 18, 2021

Choose a reason for hiding this comment

rockwalrus Jun 18, 2021

Choose a reason for hiding this comment

sokra Jun 21, 2021

Choose a reason for hiding this comment

sokra Jun 18, 2021

Choose a reason for hiding this comment

rockwalrus Jun 22, 2021

Choose a reason for hiding this comment

webpack-bot commented Jun 22, 2021

sokra commented Jun 22, 2021

rockwalrus commented Jun 22, 2021

alexander-akait commented Jun 22, 2021

rexb1971 commented Jun 24, 2021

sokra commented Jun 28, 2021

rockwalrus commented Jun 29, 2021

sokra commented Jun 30, 2021

rexb1971 commented Jun 30, 2021

rockwalrus commented Jun 30, 2021

rockwalrus commented Jul 16, 2021

alexander-akait commented Jul 16, 2021

webpack-bot commented Jul 17, 2021

sokra commented Jul 18, 2021

rockwalrus commented Jul 19, 2021

webpack-bot commented Jul 22, 2021

webpack-bot commented Jun 17, 2021 •

edited by sokra