Skip to content

Added a worker property to access the underlying worker #27

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.

Sign up for GitHub

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

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

Added a worker property to access the underlying worker #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ const comlinkLoaderSpecificOptions = [
'singleton'
];

export default function loader () { }
export default function loader() { }

loader.pitch = function (request) {
const options = loaderUtils.getOptions(this) || {};
Expand All @@ -39,30 +39,56 @@ loader.pitch = function (request) {

const remainingRequest = JSON.stringify(workerLoader + '!' + request);

const wrapperBuilder = `
var Worker = require(${remainingRequest});
var wrap = require('comlink').wrap;

function getWrappedWorker() {
var worker = Worker();
var wrapper = wrap(worker);

if (typeof Proxy === 'function') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this just use Object.defineProperty to handle both cases? (not sure if the comlink proxy traps it)

Copy link
Author

Choose a reason for hiding this comment

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

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.

var proxy = new Proxy(wrapper, {
get: function (target, prop, receiver) {
if (prop === 'worker') {
return worker;
}
return Reflect.get(...arguments);
}
});

return proxy;
} else {
wrapper.worker = worker;
return wrapper;
}
} `;


// ?singleton mode: export an instance of the worker
if (singleton === true) {
return `
module.exports = require('comlink').wrap(require(${remainingRequest})());
${options.module === false ? '' : 'module.exports.__esModule = true;'}
`.replace(/\n\s*/g, '');
${wrapperBuilder}
module.exports = getWrappedWorker();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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")

${ options.module === false ? '' : 'module.exports.__esModule = true;'}
`.replace(/\n\s*/g, '');
}

// ?singleton=false mode: always return a new worker from the factory
if (singleton === false) {
return `
module.exports = function () {
return require('comlink').wrap(require(${remainingRequest})());
};
`.replace(/\n\s*/g, '');
${wrapperBuilder}
module.exports = getWrappedWorker; `.replace(/\n\s*/g, '');
}

return `
var wrap = require('comlink').wrap,
Worker = require(${remainingRequest}),
inst;
module.exports = function f() {
if (this instanceof f) return wrap(Worker());
return inst || (inst = wrap(Worker()));
};
${ wrapperBuilder}
var inst;
module.exports = function f() {
if (this instanceof f)
return getWrappedWorker();

return inst || (inst = getWrappedWorker());
};
`.replace(/\n\s*/g, '');
};
22 changes: 19 additions & 3 deletions test/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,20 @@ import sinon from 'sinon';
import 'jasmine-sinon';
import MyWorker from 'comlink-loader!./worker';

const OriginalWorker = self.Worker;
self.Worker = sinon.spy((url, opts) => new OriginalWorker(url, opts));

describe('worker', () => {
let OriginalWorker;
let worker, inst;

beforeAll(() => {
OriginalWorker = self.Worker;
self.Worker = sinon.spy((url, opts) => new OriginalWorker(url, opts));
});

afterAll(() => {
// Reset the original Worker constructor for next tests
self.Worker = OriginalWorker;
});

it('should be a factory', async () => {
worker = new MyWorker();
expect(self.Worker).toHaveBeenCalledOnce();
Expand Down Expand Up @@ -74,4 +82,12 @@ describe('worker', () => {

expect(self.Worker).not.toHaveBeenCalled();
});

it('should have a property to access the underlying worker', async () => {
self.Worker.resetHistory();

const worker = MyWorker();
expect(worker.worker instanceof OriginalWorker).toBe(true);
});
});

19 changes: 16 additions & 3 deletions test/singleton.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,20 @@
import sinon from 'sinon';
import 'jasmine-sinon';

const OriginalWorker = self.Worker;
self.Worker = sinon.spy((url, opts) => new OriginalWorker(url, opts));

describe('singleton', () => {
let OriginalWorker;
let exported;

beforeAll(() => {
OriginalWorker = self.Worker;
self.Worker = sinon.spy((url, opts) => new OriginalWorker(url, opts));
});

afterAll(() => {
// Reset the original Worker constructor for next tests
self.Worker = OriginalWorker;
});

it('should immediately instantiate the worker', async () => {
// we're using dynamic import here so the Worker spy can be installed before-hand
exported = require('comlink-loader?singleton!./worker');
Expand Down Expand Up @@ -50,4 +58,9 @@ describe('singleton', () => {
expect(e).toMatch(/Error/);
}
});

it('should have a property to access the underlying worker', async () => {
expect(exported.worker instanceof OriginalWorker).toBe(true);
});
});