Open otariidae opened 1 year ago
I'm ok that adding identifier to messages.
Specifically, the worker code receives the id
and puts the id
against the xxx:result
as follows?
const id = crypto.randomUUID();
worker.addEventListener("message", (event) => {
if(event.data.id === id) {
console.log("result of #1");
}
})
worker.postMessage({
id, // <= added
command: "lint",
text: "text#1 short text; lint finishes quickly",
ext: ".txt"
})
Yes. The way you showed looks good to me.
Another Discussion Points
metadata
property is another option, but id
is simpler.metdata: {}
or id: string
id
is required property not to be optional?
id
as it looks like a common practice in other libraries such as comlink and typed-workerid
property to avoid the pitfall of race condition by designI agree with you.
id
to messaging as required propertyWelcome to PR
I am trying to make a PR and found another discussion point:
id
is missing or something goes wrongworker.addEventListener("error", ...)
but Error object does not have id
id
?One of my ideas is that the worker responds with a newly-defined format like TextlintWorkerCommandResponseError
instead of throwing Error.
Yeah, error
response is reasonable.
Sorry, I found a simple oversight in my idea. When id
is missing, the worker cannot respond with id
.
My idea works only if id
exists and something goes wrong, for example, when kernel.lintText
fails.
I am considering another way or someone have good idea?
My another idea is just to throw Error if id
is missing to show that the worker will never work without id
, and to respond with TextlintWorkerCommandResponseError
if id
exists and Textlint fails.
I think that worker should just post error
message when id
is missing.
It is global error like Window: error event.
If the command has id
and some error on liting, worker post error
with id
like { message: "xxx", id/commandId: "xxx" }
User always listen the error
event.
worker.addEventListener("message", (event) => {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "error") {
console.error(data.error); // missing id error
}
});
worker.postMessage({
command: "lint",
text: "text#1 short text; lint finishes quickly",
ext: ".txt"
});
const id = crypto.randomUUID();
worker.addEventListener("message", (event) => {
const data: TextlintWorkerCommandResponse = event.data;
if (data.command === "error") {
console.error(data.error, data.error.id); // something error at `id`.
}
});
worker.postMessage({
id,
command: "lint",
text: "text#1 short text; lint finishes quickly",
ext: ".txt"
});
It may be better to proceed separately with the PR to add the id
and the PR to make the id
mandatory.
The latter is a breaking change, so there is a lot to think about.
My idea that just throws Error when missing id
looks like this:
const id = crypto.randomUUID();
worker.addEventListener("message", function onResult(event) {
const data: TextlintWorkerCommandResponse = event.data;
if (data.id === id) {
if (data.error) {
console.error(data.error); // id exists but something went wrong
} else {
console.log(data.result) // succeeded
}
worker.removeEventListener("message", onResult);
worker.removeEventListener("error", onError);
}
});
worker.addEventListener("error", function onError(error) {
console.error(error); // missing id error
worker.removeEventListener("message", onResult);
worker.removeEventListener("error", onError);
});
Or more comprehensive idea is that script-compiler provides client-side code to force Textlint message protocol as some other libraries do. Though this is complicated.
const textlint = wrap(new Worker("textlint-worker.js"));
const results = await Promise.all([
textlint.lint("text#1 short text; lint finishes quickly", ".txt"),
textlint.lint("text#2 short text; lint finishes quickly", ".txt")
]):
// safe for race conditions by default
// provides best way to work with Textlint worker, id and error handling included
Ah, I did not know that catch the exception on the worker via worker.addEventListener("error", ...
.
It is better than command === "error"
.
I am considering that it may be better to first create a PR that provides error response messages for error handling, then make a PR that makes id
required.
I'd like to have a one-to-one correspondence between the text I send to the Textlint worker and the lint result I receive from the Textlint worker. However, the order I received results from the Textlint worker can differ from the one I posted texts to the worker probably because
kernel.lintText
is async. Ref:https://github.com/textlint/editor/blob/b32d016d23087d6e7e3ff8de84984a10a90e2815/packages/%40textlint/script-compiler/src/CodeGenerator/worker-codegen.ts#L125-L136
So relying on message reception order can cause a race condition. Pseudocode:
More higher-level example:
https://github.com/textlint/editor/blob/b32d016d23087d6e7e3ff8de84984a10a90e2815/packages/textchecker-element/index.ts#L41-L63
Some Web Worker libraries use IDs in messages. I think it can be one of the ideas to solve this problem. For example, comlink:
https://github.com/GoogleChromeLabs/comlink/blob/dffe9050f63b1b39f30213adeb1dd4b9ed7d2594/src/comlink.ts#L596-L615
For another example, minlink:
https://github.com/mizchi/minlink/blob/98f0eed1b1fc00a51709e07c6fe3e18232cdfaad/src/shared.ts#L52-L61