Add CORSWorkaround option + update isAbsoluteURL

[aminya]

Fixes #284
Closes #286

[aminya]

The error that I got in #284 is now changed to this:

worker.js:1 Uncaught ReferenceError: require is not defined
    at worker.js:1
(anonymous) @ worker.js:1

C:\Users\myproject\ide-json\node_modules\threads\dist\master\spawn.js:35 Uncaught (in promise) Error: Timeout: Did not receive an init message from worker after 10000ms. Make sure the worker calls expose().
    at C:\Users\myproject\ide-json\node_modules\threads\dist\master\spawn.js:35

I think now the worker is working properly, but the code is written in a Node fashion. Now probably it is a matter of testing this on another version of Atom. I noticed that Electron needs some options for enabling Node workers:
https://www.electronjs.org/docs/tutorial/multithreading

Edit:

I tested on my version of Atom that enables multi-threading and the errors are all gone! 🎉
atom-community/atom#74

[andywer]

Looking good, but we can't just drop the CORS fix like that.

Can you try reverting this change here and instead change the regex to this?

/^(blob:|file:|https?:)?\/\//i

[aminya]

I reverted the commit but the error is back! Are you sure that we need the cors protection in the second condition?

[andywer]

Yeah, we need that. But maybe this little change could already straighten things (in the WebWorker constructor)?

-  }
-  if (typeof url === "string" && isAbsoluteURL(url)) {
+  } else if (typeof url === "string" && isAbsoluteURL(url)) {

I don't have any time today, but maybe that helps already :)

[aminya]

This does not fix the issue. The code does not enter this condition at all. The problem happens in the second branch of if. This is the compiled code. It goes inside the second branch.

We need to have a better detection for when the CORS protection is needed. In this case, it is adversely affecting the code.

    class WebWorker extends Worker {
        constructor(url, options) {
            if (typeof url === "string" && options && options._baseURL) {
                url = new URL(url, options._baseURL);
            }
            else if (typeof url === "string" && !isAbsoluteURL(url) && get_bundle_url_browser_1.getBundleURL().match(/^file:\/\//i)) {
                console.log("comes here") //HERE

                url = new URL(url, get_bundle_url_browser_1.getBundleURL().replace(/\/[^\/]+$/, "/"));

				// do we need this?? this is the problem
                url = createSourceBlobURL(`importScripts(${JSON.stringify(url)});`);
            }
            if (typeof url === "string" && isAbsoluteURL(url)) {
                // Create source code blob loading JS file via `importScripts()`
                // to circumvent worker CORS restrictions
                url = createSourceBlobURL(`importScripts(${JSON.stringify(url)});`);
            }
            super(url, options);
        }
    }

[andywer]

Ahhh, you know what? I think we need to fix the isAbsoluteURL function like this:

- const isAbsoluteURL = (value: string) => /^(file|https?:)?\/\//i.test(value)
+ const isAbsoluteURL = (value: string) => /^(file:|https?:)?\/\//i.test(value) || /^blob:/.test(value)

As the blob: URL we see does not come with double slashes…

[andywer]

@aminya Any news?

[aminya]

@andywer no difference:

C:\Users\aminy\Docum…ation.browser.js:40 Refused to create a worker from 'blob:file:///6be44486-60a8-4fa6-9cf6-0b0137592712' because it violates the following Content Security Policy directive: "script-src 'self' 'unsafe-eval'". Note that 'worker-src' was not explicitly set, so 'script-src' is used as a fallback.

As I said the problem is this line:

threads.js/src/master/implementation.browser.ts

Line 37 in 7bf8ed9

url = createSourceBlobURL(`importScripts(${JSON.stringify(url)});`)

[andywer]

As I said the problem is this line:

Sure, but it should not execute that line in the first place as the condition for that else if branch contains !isAbsoluteURL(url) and I would say a blob URL qualifies as absolute URL…

So weird.

[aminya]

@andywer But the input url is ./worker.js. It is not an absolute URL, so it goes inside the second condition.

Why not use a package like https://www.npmjs.com/package/is-absolute-url, which covers all the possibilities.

[andywer]

Ahhh, that happens when you don't run the code yourself… Of course, you are right!

Why not use a package like https://www.npmjs.com/package/is-absolute-url, which covers all the possibilities.

Could do, but I also try to find a balance where we don't rely on too many dependencies. So for these classic one-liners I try to avoid introducing additional deps. But yeah, maybe those one-liners should be based more closely on existing code 😉

C:\Users\aminy\Docum…ation.browser.js:40 Refused to create a worker from 'blob:file:///6be44486-60a8-4fa6-9cf6-0b0137592712' because it violates the following Content Security Policy directive: "script-src 'self' 'unsafe-eval'". Note that 'worker-src' was not explicitly set, so 'script-src' is used as a fallback.

What is your Content Security Policy, btw?

We have been using this CSP in an Electron/Cordova app:

<meta http-equiv="Content-Security-Policy" content="default-src 'self'; child-src 'self' blob: file://*; connect-src *; img-src *; script-src 'self' blob: file://*; style-src 'self' 'unsafe-inline'; worker-src 'self' blob: file://*;">

[aminya]

Ahhh, that happens when you don't run the code yourself… Of course, you are right!

So what is the solution now? Should we use a better is-absolute-url function?

Why not use a package like npmjs.com/package/is-absolute-url, which covers all the possibilities.

Could do, but I also try to find a balance where we don't rely on too many dependencies. So for these classic one-liners I try to avoid introducing additional deps. But yeah, maybe those one-liners should be based more closely on existing code 😉

Using tree-shaking and ~version in npm, the downsides of having dependency go away. But I understand that you may want to keep package.json smaller. 😃

The benefit of these libraries is that they are well tested and they usually promise to do this simple functionality well!

What is your Content Security Policy, btw?

We have been using this CSP in an Electron/Cordova app:

<meta http-equiv="Content-Security-Policy" content="default-src 'self'; child-src 'self' blob: file://*; connect-src *; img-src *; script-src 'self' blob: file://*; style-src 'self' 'unsafe-inline'; worker-src 'self' blob: file://*;">

Atom's CSP is this:
https://github.com/atom/atom/blob/2f827bfc23260eece024b8f179421fa833b1834d/static/index.html#L4

[andywer]

Let's improve the function we have, so that it matches /^[A-Za-z0-9]+:/ and /^\/\//. But as you pointed out, that won't fix the issue.

I take it you are working with Atom and cannot change the CSP? 🙈

So if the code works when this line of code is not run, then let's just add an option to opt-out of the CORS workaround.

[aminya]

@andywer I added the option. Should it be documented anywhere? I don't know where to add the docs.

[aminya]

This is ready to go too.

[andywer]

Great stuff! 👍

Should it be documented anywhere?

Maybe, but as it's quite specific and hopefully not going to be needed too often, I think we can live with only documenting the code inline (also shown in IntelliSense) for now.