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

Set operation is not awaitable #638

Open
ugudango opened this issue Apr 17, 2023 · 2 comments
Open

Set operation is not awaitable #638

ugudango opened this issue Apr 17, 2023 · 2 comments

Comments

@ugudango
Copy link

ugudango commented Apr 17, 2023

I'm currently working on something that has parts similar to comlink, and I read through the code, as to not reinvent the wheel.

I found this piece of code, which has been marked as FIXME.

comlink/src/comlink.ts

Lines 476 to 490 in dffe905

set(_target, prop, rawValue) {
throwIfProxyReleased(isProxyReleased);
// FIXME: ES6 Proxy Handler `set` methods are supposed to return a
// boolean. To show good will, we return true asynchronously ¯\_(ツ)_/¯
const [value, transferables] = toWireValue(rawValue);
return requestResponseMessage(
ep,
{
type: MessageType.SET,
path: [...path, prop].map((p) => p.toString()),
value,
},
transferables
).then(fromWireValue) as any;
},

This code causes problems. set has to return boolean because you can't await the assignment.

Unfortunately, there's nothing like this in ES currently, and there probably won't be for the foreseeable future:

const variable (await =) 'some value';

I even did some tests on awaiting the entire expression itself, and it is indeed not trackable in any way.

You cannot chain operations if you depend on setting a value through a proxy. Of course, there's always the option of endpoints, but there is a viable alternative to the current implementation, and it would make this use case possible.

Moreover, it is not entirely clear that this should not work as expected.

There are two possible approaches to this issue:

  1. Export a unique symbol that would "end" the chain to an assignment. This would translate in adding an extra "if" case in the get interface.
  2. On top of that, use ts-morph (or maybe even a JS AST manipulation library) to compile the code, so that the normal assignment translates.

Option 1. would make the code look like this:

import {set} from 'comlink'; // this is the symbol

await myProxy.foo.bar[set](3); // this is now awaitable

Option 2. would keep the code as it currently is, but would require an extra compilation step plus a babel/ts-morph/rollup/unplugin/etc. implementation.

This would be a big design change in terms of DX, but it would change the library for the better, I think.

I can provide a PR with tests as well, just let me know if this is something you're interested in.

@benjamind
Copy link
Collaborator

I am hesitant to introduce an extra compilation step, especially if that results in specific build time configurations on the part of the consuming developer. Thus far this doesn't appear to have been a major issue for folks, so I raise the question, does it need fixing at all?

@ugudango
Copy link
Author

I think it may be at most worth a mention in the README. It's really easy to find an alternative (just expose a function if you care about the order of operations), so I'm guessing people would've just switched to that had they ever encountered this edge case.

The only reason this can be confusing is because Comlink has a very friendly DX. Following that pattern, some users may just assume things. As you said though, people haven't complained, but it's still a useful piece of info IMO.

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

No branches or pull requests

2 participants