GoogleChromeLabs / comlink

Comlink makes WebWorkers enjoyable.
Apache License 2.0
11k stars 382 forks source link

Set operation is not awaitable #638

Open ugudango opened 1 year ago

ugudango commented 1 year ago

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.

https://github.com/GoogleChromeLabs/comlink/blob/dffe9050f63b1b39f30213adeb1dd4b9ed7d2594/src/comlink.ts#L476-L490

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 commented 6 months ago

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 commented 6 months ago

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.