Added a worker property to access the underlying worker
#27
Conversation
Fixed the setup and tear down code in tests
| var worker = Worker(); | ||
| var wrapper = wrap(worker); | ||
|
|
||
| if (typeof Proxy === 'function') { |
There was a problem hiding this comment.
Could this just use Object.defineProperty to handle both cases? (not sure if the comlink proxy traps it)
There was a problem hiding this comment.
Just a couple questions, otherwise this looks good and the tests are a welcome addition.
I'm mainly concerned with making sure the Proxy usage doesn't reduce browser support - I'm guessing since Comlink already relies on Proxy it's okay?
I'm not that familiar with proxies (I actually discovered them when looking at comlink's code) but from what I understood it traps all attempts to access properties and thus to get a result it has to be defined as a property in the proxied object before creating the proxy or have a custom 'resolver' for a custom property key (as I did in comlink with the 'terminate' method).
In our case, the wrap function creates and returns a Proxy object over our worker and thus can't provide a '.worker' property accessor as that would mean either the wrapped object (the worker) has a 'worker' property or the proxy object is created with a resolver for 'worker' which would return the proxied object and would, in this case, break the sandboxing contract offered by the Proxy.
So the only solution I found at the time was to proxy the proxy so I could add my custom resolver.
I just tested with:
return Object.defineProperty(wrapper, 'worker', {value: worker});
and a dirty hack:
var worker = Worker();
worker = Object.defineProperty(worker, 'worker', {value: worker});
var wrapper = wrap(worker);
return wrapper;
The unit tests are failing in both cases (glad I wrote them :-)) with :
TypeError: 'get' on proxy: property 'worker' is a read-only and non-configurable data property on the proxy target but the proxy did not return its actual value (expected '#' but got '[object Function]')
Not sure what to do with this error and I found a couple of occurrences of the same error on other projects and StackOverflow.
As to the browser support, as you said, the first thing comlink does is create a proxy, so I'm not adding any more constraint. According to https://caniuse.com/?search=Proxy only IE11 and the Baidu Browser 7.12 (released in 2017) do support Web Workers but not Proxy, but I guess as official support of IE11 has been dropped and Baidu Browser seems to be now in version 43 and running on Chromium it should be totally fine to assume we're not causing more harm.
| ${options.module === false ? '' : 'module.exports.__esModule = true;'} | ||
| `.replace(/\n\s*/g, ''); | ||
| ${wrapperBuilder} | ||
| module.exports = getWrappedWorker(); |
There was a problem hiding this comment.
In the singleton case it seems like there might not be a need for terminating the Worker since it's a side effect of module Instantiation. Thoughts?
There was a problem hiding this comment.
I don't use the singleton mode myself, but I thought it would be nice to have a consistent API and it allowed me to factor the code.
There is one use case that I may think of. in the context of a SPA, one might want to terminate all singletons for various workers when the user logs out, to avoid potential access to data of the now gone user inside the worker once another user signs in to the app (ie. view it as a singleton for the lifetime of the user session as opposed to the lifetime of the browser tab/window). but as we say it in French: c'est tiré par les cheveux. (it's far fetched, literally "it's dragged by the hair")
As discussed in GoogleChromeLabs/comlink#487, this is a proposal to implement a worker property to the wrapper returned by this loader. This allows the user to properly terminate the Worker.
I also fixed the setup and tear down code in tests as the spy put in place of the original worker was kept by from one test suite to another and made further tests fail.