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

Allow HMR status handlers to return a Promise #13576

Merged
merged 2 commits into from
Jul 18, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 66 additions & 60 deletions lib/hmr/HotModuleReplacement.runtime.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ var $interceptModuleExecution$ = undefined;
var $moduleCache$ = undefined;
// eslint-disable-next-line no-unused-vars
var $hmrModuleData$ = undefined;
/** @type {() => Promise} */
var $hmrDownloadManifest$ = undefined;
var $hmrDownloadUpdateHandlers$ = undefined;
var $hmrInvalidateModuleHandlers$ = undefined;
Expand Down Expand Up @@ -209,8 +210,12 @@ module.exports = function () {

function setStatus(newStatus) {
currentStatus = newStatus;
var results = [];

for (var i = 0; i < registeredStatusHandlers.length; i++)
registeredStatusHandlers[i].call(null, newStatus);
results[i] = registeredStatusHandlers[i].call(null, newStatus);

return Promise.all(results);
}

function trackBlockingPromise(promise) {
Expand All @@ -219,7 +224,7 @@ module.exports = function () {
setStatus("prepare");
blockingPromises.push(promise);
waitForBlockingPromises(function () {
setStatus("ready");
return setStatus("ready");
});
return promise;
case "prepare":
Expand All @@ -243,47 +248,47 @@ module.exports = function () {
if (currentStatus !== "idle") {
throw new Error("check() is only allowed in idle status");
}
setStatus("check");
return $hmrDownloadManifest$().then(function (update) {
if (!update) {
setStatus(applyInvalidatedModules() ? "ready" : "idle");
return null;
}

setStatus("prepare");

var updatedModules = [];
blockingPromises = [];
currentUpdateApplyHandlers = [];

return Promise.all(
Object.keys($hmrDownloadUpdateHandlers$).reduce(function (
promises,
key
) {
$hmrDownloadUpdateHandlers$[key](
update.c,
update.r,
update.m,
promises,
currentUpdateApplyHandlers,
updatedModules
);
return promises;
},
[])
).then(function () {
return waitForBlockingPromises(function () {
if (applyOnUpdate) {
return internalApply(applyOnUpdate);
} else {
setStatus("ready");
return setStatus("check")
.then($hmrDownloadManifest$)
.then(function (update) {
if (!update) {
return setStatus(applyInvalidatedModules() ? "ready" : "idle");
}

return updatedModules;
}
return setStatus("prepare").then(function () {
var updatedModules = [];
blockingPromises = [];
currentUpdateApplyHandlers = [];

return Promise.all(
Object.keys($hmrDownloadUpdateHandlers$).reduce(function (
promises,
key
) {
$hmrDownloadUpdateHandlers$[key](
update.c,
update.r,
update.m,
promises,
currentUpdateApplyHandlers,
updatedModules
);
return promises;
},
[])
).then(function () {
return waitForBlockingPromises(function () {
if (applyOnUpdate) {
return internalApply(applyOnUpdate);
} else {
return setStatus("ready").then(function () {
return updatedModules;
});
}
});
});
});
});
});
}

function hotApply(options) {
Expand Down Expand Up @@ -312,21 +317,20 @@ module.exports = function () {
.filter(Boolean);

if (errors.length > 0) {
setStatus("abort");
return Promise.resolve().then(function () {
return setStatus("abort").then(function () {
throw errors[0];
});
}

// Now in "dispose" phase
setStatus("dispose");
var disposePromise = setStatus("dispose");

results.forEach(function (result) {
if (result.dispose) result.dispose();
});

// Now in "apply" phase
setStatus("apply");
var applyPromise = setStatus("apply");

var error;
var reportError = function (err) {
Expand All @@ -345,25 +349,27 @@ module.exports = function () {
}
});

// handle errors in accept handlers and self accepted module load
if (error) {
setStatus("fail");
return Promise.resolve().then(function () {
throw error;
});
}
return Promise.all([disposePromise, applyPromise]).then(function () {
// handle errors in accept handlers and self accepted module load
if (error) {
return setStatus("fail").then(function () {
throw error;
});
}

if (queuedInvalidatedModules) {
return internalApply(options).then(function (list) {
outdatedModules.forEach(function (moduleId) {
if (list.indexOf(moduleId) < 0) list.push(moduleId);
if (queuedInvalidatedModules) {
return internalApply(options).then(function (list) {
outdatedModules.forEach(function (moduleId) {
if (list.indexOf(moduleId) < 0) list.push(moduleId);
});
return list;
});
return list;
});
}
}

setStatus("idle");
return Promise.resolve(outdatedModules);
return setStatus("idle").then(function () {
return outdatedModules;
});
});
}

function applyInvalidatedModules() {
Expand Down
3 changes: 2 additions & 1 deletion test/HotTestCases.template.js
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ const describeCases = config => {
return JSON.parse(fs.readFileSync(p, "utf-8"));
} else {
const fn = vm.runInThisContext(
"(function(require, module, exports, __dirname, __filename, it, beforeEach, afterEach, expect, self, window, fetch, document, importScripts, Worker, EventSource, NEXT, STATS) {" +
"(function(require, module, exports, __dirname, __filename, it, beforeEach, afterEach, expect, jest, self, window, fetch, document, importScripts, Worker, EventSource, NEXT, STATS) {" +
"global.expect = expect;" +
'function nsObj(m) { Object.defineProperty(m, Symbol.toStringTag, { value: "Module" }); return m; }' +
fs.readFileSync(p, "utf-8") +
Expand All @@ -271,6 +271,7 @@ const describeCases = config => {
_beforeEach,
_afterEach,
expect,
jest,
window,
window,
window.fetch,
Expand Down
11 changes: 6 additions & 5 deletions test/hotCases/invalidate/during-idle/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@ it("should allow to invalidate and reload a file", () => {
expect(module.hot.status()).toBe("ready");
c.invalidate();
expect(module.hot.status()).toBe("ready");
module.hot.apply();
expect(module.hot.status()).toBe("idle");
expect(a.value).not.toBe(oldA);
expect(b.value).not.toBe(oldB);
expect(c.value).toBe(oldC);
module.hot.apply().then(function () {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

expect(module.hot.status()).toBe("idle");
expect(a.value).not.toBe(oldA);
expect(b.value).not.toBe(oldB);
expect(c.value).toBe(oldC);
});
});
3 changes: 3 additions & 0 deletions test/hotCases/status/accept/file.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = 1;
---
module.exports = 2;
27 changes: 27 additions & 0 deletions test/hotCases/status/accept/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
var value = require("./file");

it("should wait until promises returned by status handlers are fulfilled", (done) => {
var handler = jest.fn(status => {
var test = jest.fn(() => {
expect(module.hot.status()).toBe(status == "dispose" ? "apply" : status);
});

var promise = Promise.resolve().then(test);
promise.test = test;

return promise;
});
module.hot.addStatusHandler(handler);
module.hot.accept("./file", () => {
value = require("./file");
});
NEXT(require("../../update")(done, undefined, () => {
expect(handler.mock.calls).toStrictEqual([['check'], ['prepare'], ['dispose'], ['apply'], ['idle']]);
for (let result of handler.mock.results)
expect(result.value.test).toHaveBeenCalledTimes(1);

expect(module.hot.status()).toBe("idle");

done();
}));
});