fix(messaging)!: Fix mix up messaging in window and custom event messengers #70
Conversation
There was a problem hiding this comment.
Switch page.test to browser mode test.
| res(detail); | ||
| if ( | ||
| detail.namespace === namespace && | ||
| detail.instanceId !== instanceId && | ||
| detail.message.type === requestEvent.detail.message.type | ||
| ) { | ||
| res(detail.response); | ||
| } |
There was a problem hiding this comment.
Is it a mistake that unlike windowMessage, customEventMessage has been unguarded so far? Is there a particular reason I don't know about?
There was a problem hiding this comment.
Probably a bug, but could be related to how window.postMessage sends the message to all frames in the window.
| removeAdditionalListeners.push(() => | ||
| window.removeEventListener(RESPONSE_EVENT, responseListener), | ||
| ); | ||
| window.addEventListener(RESPONSE_EVENT, responseListener, { once: true }); |
There was a problem hiding this comment.
Here, too, I don't understand why it has been once: true so far.
|
@aklinker1 PR is ready for review. But there's really no need hurry. Because at least in my project, I'm resuming work on this content with a local patching. 😃 |
| const reqDetail = { message, namespace, instanceId }; | ||
| const requestEvent = new CustomEvent(REQUEST_EVENT, { | ||
| // @ts-expect-error not exist cloneInto types because implemented only in Firefox. | ||
| detail: typeof cloneInto !== 'undefined' ? cloneInto(reqDetail, window) : reqDetail, |
There was a problem hiding this comment.
There is a problem with CustomEvent in Firefox that causes an error when sharing a message object.
|
Waiting for merge.... |
|
No problem, thanks for the hard review! |
|
Released in v2.0.0 |
|
@aklinker1 It seems that the published |
|
Thanks, confirmed that the correct update came 🚀 |
fix #57
Firstly
It may be hard to review, so take your time.
Also, if approved, it might be safer to make it a major bump. Because the messaging path is narrowed and pseudo-channeled, it is possible that a user's application, through which the message previously happened to pass, could be broken.
Changes
message
First, has a unique instance ID. This is to check whether the message came from its own instance or not. Then,
namespace,instanceId, andmessage typeare checked strictly in the req/res listeners.messaging-demo
About vitest
browser modeI was annoyed by the flakey test in
page.test, so I partially switched tobrowser mode. I understand that thewindow(jsdom global context) on nodeVM is common between tests, but can be made isolated when inbrowser mode.I really wanted to split the test environment nicely with workspace configs like in this example, but I gave up because of a bug that broke the existing unit test. The bug that cjs mocking (
webextension-polyfill) fails when using browser mode together. They seem to have reworked the whole mock mechanism.This Issue (#48) may have been resolved. But the link is already dead, I don't know which test Flakey was testing at the time.
Diagrams
Here is the understanding I have in my head and its diagram.
sequenceDiagram box Content-Script<br/><br/>instance1<br/>{ namespace: foo } participant ins1_on as onMessage participant ins1_send as sendMessage end box Injected/WebSite<br/><br/>instance2<br/>{ namespace: foo } participant ins2_on as onMessage participant ins2_send as sendMessage end box AnotherContext<br/><br/>instance3<br/>{ namespace: bar } participant ins3_on as onMessage participant ins3_send as sendMessage end %% namespace B Note over ins3_on,ins3_send: COMMUNICATION FLOW<br/>SAME AS LEFT par ins1_send->>+ins2_on: send/type:"ping1" ins1_send--xins1_on: ignore own send ins2_on--xins2_send: ignore own res ins2_on->>-ins1_send: res/type:"ping1" end par ins2_send->>+ins1_on: send/type:"ping2" ins2_send--xins2_on: ignore own send ins1_on--xins1_send: ignore own res ins1_on->>-ins2_send: res/type:"ping2" end %% no communicate lines Note over ins2_on,ins3_send: No communicate in<br/>different namespaces