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

Add ability to force main thread fallback for workers #337

Closed
paularden-onshape opened this issue Nov 6, 2024 · 6 comments
Closed

Add ability to force main thread fallback for workers #337

paularden-onshape opened this issue Nov 6, 2024 · 6 comments

Comments

@paularden-onshape
Copy link

Due to the issue described in #31 and the check that is used for falling back to the main thread it is currently not possible to avoid console errors for CSP policy violations and triggering CSP policy violations.

This becomes a greater issue for those using restrictive CSP in combination with reporting by setting the report-to or report-uri CSP directives (see here for more details). In this configuration each time the page is visited the CSP violation will be reported to that URL.

For users who know that they will not be able to provide a permissive enough CSP, it would be desirable to have a mechanism to force the fallback to be used without the check so that the fallback is always used. That would ideally also suppress any warning logs since the user has explicitly chosen to use the fallback.

@lojjic
Copy link
Collaborator

lojjic commented Nov 6, 2024

I like the idea. Shouldn't be too hard to implement.

@lojjic lojjic closed this as completed in 86023ec Nov 10, 2024
@lojjic
Copy link
Collaborator

lojjic commented Nov 10, 2024

Done, you can configure it like so:

import {configureTextBuilder} from 'troika-three-text'

configureTextBuilder({
  useWorker: false
})

@paularden-onshape
Copy link
Author

@lojjic thanks for the rapid response. I'm just trying this out and am having a few issues. Firstly, the change doesn't actually prevent the CSP violation (and therefore triggers the call to the report-uri) since defineWorkerModule still does the test to see if the worker can be made, regardless of the configuration.

In that failure case it now also breaks the fallback since it returns onMainThread directly where as it now gets used with the configuration check by looking for .onMainThread on the object that is returned from defineWorkerModule. So that return onMainThread should probably be return { onMainThread } to fix that.

However, what would be more ideal is if defineWorkerModule was passed an (optional) option, say useFallback like the config so that the test could become:

if (useFallback || !supportsWorkers()) {
  return { onMainThread }
}

The useFallback option would just be passed in from in TextBuilder.js as !CONFIG.useWorker, so it can be optional and falsey values wouldn't do anything.

Did you want me to make a separate issue for this?

@lojjic
Copy link
Collaborator

lojjic commented Nov 11, 2024

Ahh, good points. I obviously didn't test this out with a CSP in place. I'll reopen and fix.

@lojjic lojjic reopened this Nov 11, 2024
@lojjic lojjic closed this as completed in d396e51 Nov 11, 2024
@lojjic
Copy link
Collaborator

lojjic commented Nov 11, 2024

Followup fix is published. I've tested it with all 4 scenarios: CONFIG.useWorkers true+false, and browser worker support true+false.

I ended up just moving the supportsWorkers check from module definition-time to module function call-time, and ditched the early return.

Unfortunately your idea of passing a useFallback flag to the createWorkerModule config wouldn't work, because it also needs to force any dependency workermodules to use the main thread, and those have already been defined so it's too late to pass that flag along to them. 😉

@paularden-onshape
Copy link
Author

Thanks, looks to be working for me now.

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

No branches or pull requests

2 participants