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

Improve two-way communication test #621

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Nemikolh
Copy link

Hello! πŸ‘‹

TLDR;

This PR improves the two-way communication test to fail if WireValueType and MessageType would happen to overlap again in the future.

Full story

First, I want to thank you for your work on Comlink. This is one of the most elegant, well written library I've ever come across ⭐ πŸ‘

I recently noticed that the test for two-way communication does not do enough work to actually fail in versions of Comlink < 4.3.1. I encountered this issue while trying to port two-way communication to our fork which is stuck at 4.3.0.

To see how this PR helps you can use this bash script:
# To run this, you need jq, npm, git

# Checkout this branch
BRANCH=fix-two-way-iframe-test
git remote add Nemikolh git@github.com:Nemikolh/comlink.git
git fetch Nemikolh
git checkout $BRANCH || { echo "Oopsie"; exit 1; }

# Hacky way to keep our test around
mkdir -p tmp/fixtures
cp tests/tsconfig.json                  tmp/
cp tests/type-checks.ts                 tmp/
cp tests/two-way-iframe.comlink.test.js tmp/
cp tests/fixtures/two-way-iframe.html   tmp/fixtures/
cp package.json package.json.tmp
cp tsconfig.json tsconfig.json.tmp
cp rollup.config.mjs rollup.config.mjs.tmp

# Checkout v4.3.0
git checkout v4.3.0 || { echo "Oopsie"; exit 1; }

# Make sure we run our latest tests
rm -rf tests && mv tmp tests
mv package.json.tmp      package.json
mv rollup.config.mjs.tmp rollup.config.mjs && rm rollup.config.js
mv tsconfig.json.tmp     tsconfig.json

# Use older TypeScript version (otherwise build fails)
echo "$(jq '.devDependencies.typescript = "4.6.4"' package.json)" > package.json

# Run those tests on v4.3.0
npm install
npm test

# Cleanup
rm rollup.config.mjs
git clean -f tests
git restore tests
git restore package.json
git restore tsconfig.json
git restore rollup.config.js

# Move back to the branch and run the tests again
git checkout $BRANCH
npm install
npm test

If you run this script you'll see this error in your console (you might need to scroll up a bit πŸ“œ):

AssertionError: expected DOMException{ 
     stack: 'Error: Failed to execute \'postMessage\' on \'Window\': 
                 A MessagePort could not be cloned because it was not transferred.\n   
      at Object.postMessage (dist/esm/comlink.mjs:240:48)\n    
      at dist/esm/comlink.mjs:126:16' 
} to equal ''
at Context.<anonymous> (tests/two-way-iframe.comlink.test.js:61:22)

If you run the script again but replace BRANCH=fix-two-way-iframe-test by origin/main (or equivalent), you can see that the test pass (you'll need to scroll up again to view the result of the first karma start):

23 02 2023 19:46:55.221:INFO [launcher]: Starting browser ChromeHeadless
23 02 2023 19:46:55.479:INFO [Chrome Headless 110.0.5481.100 (Linux x86_64)]: Connected on socket MGZ7B5cMMyiLbvGDAAAB with id 54251662
Chrome Headless 110.0.5481.100 (Linux x86_64): Executed 1 of 1 SUCCESS (0.019 secs / 0.005 secs)

This shows that the test is missing a few bits.

This commit improves the test to capture clone errors that occured in v4.3.1
(or lower) when trying to do two-way communication.

To do this, we expose a NonCloneable object that contains a MessagePort.
In v4.3.1 when `proxy` is called, the listener attached by `Comlink.expose`
also gets called by the remote and does not perform an early return.

Instead it attempt to do a postMessage with the raw value set to the
NonCloneable instance.

This throw an unhandledrejection.

With this new test we verify that this does not happen.
Comment on lines -37 to -38
PROXY = "PROXY",
THROW = "THROW",
Copy link
Author

Choose a reason for hiding this comment

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

I've removed those because they are unused. If you'd rather keep them, let me know and I will update my PR.

Comment on lines +58 to +59
expect(await proxy(1, 3)).to.equal(5);
// await new Promise((res) => res());
Copy link
Author

Choose a reason for hiding this comment

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

I tried to replace expect(await proxy(1, 3)).to.equal(5); with a simple await on an empty promise but that's not enough to see the error. I'm not exactly sure why.

let called = false;
let notcalled = true;
let error = "";
window.addEventListener("unhandledrejection", (ev) => {
Copy link
Author

Choose a reason for hiding this comment

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

That's how we get notified in this test that Comlink try to do a postMessage (from the Comlink.expose event listener) that it shouldn't (but it does in v4.3.0 because of the overlap between WireValueType and MessageType).

In order to get an exception, we're forcing here Comlink to do a postMessage on an object that can't be cloned.

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.

None yet

1 participant