Skip to content

worker: fix postMessage#558

Merged
saghul merged 1 commit intomasterfrom
fix-worker-postmessage
Jun 27, 2024
Merged

worker: fix postMessage#558
saghul merged 1 commit intomasterfrom
fix-worker-postmessage

Conversation

@saghul
Copy link
Copy Markdown
Owner

@saghul saghul commented Jun 27, 2024

It accepts a single argument.

Fixes: #556

It accepts a single argument.

Fixes: #556
postMessage(...args) {
this[kWorker].postMessage(args);
postMessage(message) {
this[kWorker].postMessage(message);
Copy link
Copy Markdown
Contributor

@EmixamPP EmixamPP Jun 27, 2024

Choose a reason for hiding this comment

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

I also found this morning that this was the problem when a message is sent to the worker.
The only downside of the proposed fix, is that, if a lib wants to make use of the transfer feature, the call will fail because you only accept one argument. The fact that this feature is not implemented is not a big problem, because it is just related to scope and gc I guess.
Consequently, just in case, I would add the second argument, even if not used.
I was currently elaborating that solution, but also a "better" test for the Worker, that check that indeed the message is well transmitter and received (with an Worker that do an echo)

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

It won't fail, the argument will be ignored. You can pass as many parameters as you want.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let me know if I should create a PR with what I propose or not

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It won't fail, the argument will be ignored. You can pass as many parameters as you want.

Ok fine, good to know!

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Let me know if I should create a PR with what I propose or not

Adding transfer list support requires structured cloning, which is alas not as straightforward as I thought.

For now this should do, but I'd like have transfer support in the future.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah I won't be able to implement that 😅

@EmixamPP
Copy link
Copy Markdown
Contributor

That fix works for me

@saghul saghul merged commit 1b1cfcf into master Jun 27, 2024
@saghul saghul deleted the fix-worker-postmessage branch June 27, 2024 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeError: 'this' is expected an EventTarget object, but got another value.

2 participants