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

Consider removing expose(), call() and kill() ? #26

Open
ribizli opened this issue Mar 20, 2018 · 4 comments
Open

Consider removing expose(), call() and kill() ? #26

ribizli opened this issue Mar 20, 2018 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ribizli
Copy link

ribizli commented Mar 20, 2018

	worker.expose = methodName => {
		worker[i] = function() {
			return worker.call(methodName, [].slice.call(arguments));
		};
	};

Instead methodName parameter should be used:

	worker.expose = methodName => {
		worker[methodName] = function() {
			return worker.call(methodName, [].slice.call(arguments));
		};
	};

Beside that I don't see why the call and expose methods are accessible from outside. (Don't assign to worker at all)
The purpose of the kill method is also not clear for me.

developit added a commit that referenced this issue Apr 5, 2018
@developit
Copy link
Owner

heh - i was a mistake, but it still worked for automatic methods because it was using i from the outer scope.

call and expose are both exposed because sometimes autogeneration could fail. It might be worth looking into the byte cost for this, though - I'm not sure they are widely used. Kill is just a way to shut down the worker before terminating it, and probably worth applying the same scrutiny as the other two.

@developit developit added enhancement New feature or request help wanted Extra attention is needed labels Apr 5, 2018
@developit developit changed the title expose uses closure variable i Consider removing expose(), call() and kill() ? Apr 5, 2018
@vano21
Copy link

vano21 commented Apr 14, 2018

Is it possible to terminate a worker from the main app and if so how? Calling terminate() or kill() doesn't work. Thanks.

@developit
Copy link
Owner

terminate() should work.

@boblauer
Copy link
Contributor

A fix for terminate() not working - #28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants