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

Allow accessing un-wrapped worker instance #138

Merged
merged 6 commits into from
May 26, 2024

Conversation

carlobeltrame
Copy link
Contributor

@carlobeltrame carlobeltrame commented May 16, 2024

Fixes #111, by allowing the following:

const proxy = new ComlinkWorker(new URL('./my.worker.js', import.meta.url))
proxy[Symbol.for('endpoint')].terminate()

Which gets transformed into the following code (whitespace adjusted for readability):

const proxy = ((function() {
  const endpoint = new ComlinkWorker(
    new URL('internal:comlink:./my.worker.js', import.meta.url)
  );
  const wrapped = wrap(endpoint);
  return new Proxy(wrapped, {
    get(target, prop, receiver) {
      if (prop === Symbol.for('endpoint')) return endpoint;
      return Reflect.get(...arguments);
    }
  });
})())
proxy[Symbol.for('endpoint')].terminate()

I named the property endpoint to take into account that in the case of shared workers, it's not a worker but a port instead, and Comlink calls this Worker|MessagePort hybrid type "endpoint".

@mathe42
Copy link
Owner

mathe42 commented May 16, 2024

Thanks for the PR.

I have some issues with the change

  1. The name endpoint is bad. We should use a symbol instead maybe with Symbol.for.
  2. As wrap(endpoint) is a Proxy wrapped.endpoint = endpoint; will not work as expected to properly implement this we would need to wrap wrap(endpoint) in a new Proxy. (Problem 1: I'm not convinced that this code works; Problem 2: A non breaking change in comlink could break this)

@carlobeltrame
Copy link
Contributor Author

Thanks for the quick reply! I implemented the changes you requested: It now uses Symbol.for('endpoint') for accessing the endpoint, and it now wraps the Comlink Proxy in another proxy to make sure the access to the endpoint isn't intercepted by the Comlink Proxy.
I also did a rudimentary local test of this version, by building the package and copying it into a Vite project where I logged the Worker instance and used the terminate() function on it. It worked on my machine. Let me know if I can provide any proof or unit test or reproduction of it working.

However, I don't necessarily need this code to be the final solution. This PR is just a proposal to get the ball rolling. If you have any other ideas how to solve this, I'm all for it.

@mathe42
Copy link
Owner

mathe42 commented May 17, 2024

That looks good. I will test it and release it.

@mathe42
Copy link
Owner

mathe42 commented May 26, 2024

Had to change the implementation so we get full TS support.

@mathe42 mathe42 merged commit b730c27 into mathe42:main May 26, 2024
3 checks passed
@carlobeltrame carlobeltrame deleted the patch-1 branch May 27, 2024 18:22
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.

Access to un-wrapped worker instance
2 participants