protectwise / troika

A JavaScript framework for interactive 3D and 2D visualizations
MIT License
1.65k stars 124 forks source link

Add ability to force main thread fallback for workers #337

Closed paularden-onshape closed 2 weeks ago

paularden-onshape commented 3 weeks ago

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 commented 3 weeks ago

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

lojjic commented 2 weeks ago

Done, you can configure it like so:

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

configureTextBuilder({
  useWorker: false
})
paularden-onshape commented 2 weeks ago

@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 commented 2 weeks ago

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

lojjic commented 2 weeks ago

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 commented 2 weeks ago

Thanks, looks to be working for me now.