[go: nahoru, domu]
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.

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

Jump to bottom

cannot initialize multiple canvases when using workers #107

Open
catdad opened this issue Mar 6, 2020 · 5 comments
Open

cannot initialize multiple canvases when using workers #107

catdad opened this issue Mar 6, 2020 · 5 comments
Labels
bug Something isn't working

Comments

@catdad
Copy link
Owner
catdad commented Mar 6, 2020

Currently, workers only accept a single canvas, so multiple instances cannot be used along with useWorker.

Steps:

  1. update the "custom canvas" demo to use workers
  2. start the "school pride" demo
  3. start the "custom canvas" demo

Expected: both demos work in their own canvases
Actual: once the second demo is started, the first demo starts populating confetti in the custom canvas

image
@catdad catdad added the bug Something isn't working label Mar 6, 2020
@Tidwell
Copy link
Contributor
Tidwell commented Sep 26, 2020

Just curious if there is a branch with this, I'm going to need to fix this bug for a project I'm currently working on, and wondering if theres a starting point I could use and submit back a PR for.

@catdad
Copy link
Owner Author
catdad commented Sep 27, 2020

There's no branch for this yet. I would probably try to just create a new worker for each instance that is created, and see how that works out. Let me know if you run into any trouble

@Tidwell
Copy link
Contributor
Tidwell commented Sep 27, 2020 •
edited
Loading

Yea, that was my first instinct instead of trying to get the worker to have multiple canvas instances inside it. I wasn't sure if it's better to attach the worker to the canvas element or to use a Map to store them.

Tidwell#2
Tidwell#3

They both work fine, and because the workers require transferControlToOffscreen, it has the same support matrix as Map, so I don't think that really matters. (see: https://caniuse.com/?search=transferControlToOffscreen & https://caniuse.com/?search=javascript%20Map )

Theres something to be said for not wanting to store the worker on the canvas element (which is already stated to be out of the users control once workers are enabled), though it would expose it on the canvas so the user could terminate it if they remove the canvas element.

By storing in the Map, its not polluting the DOM element, but I'm not sure if there are any hooks exposed that would allow for the user to destroy the worker.

Either one seems sufficient for my purposes, so if you have a preference, I can add tests to whichever version and get a PR put up.

@catdad
Copy link
Owner Author
catdad commented Sep 27, 2020

Hmm... I have been meaning to move over a few things into a WeakMap, but that's too much scope creep for now and can happen later.

I guess currently, I have a slight preference for storing it on the canvas object, due to memory concerns. And speaking of memory concerns (I know this is not a review yet), we'll now want to make sure to call worker.terminate() whenever a confetti instance is destroyed, so we don't leak a bunch of workers (I know some users have React cases where they'll create a new instance every time their component mounts).

@yornaath
Copy link

Yeah this is an issue when working with react atm. Any fix for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@yornaath @Tidwell @catdad