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 adding self.location to the worker global scope. #1521

Closed
johnspurlock opened this issue Jan 4, 2024 · 11 comments
Closed

Consider adding self.location to the worker global scope. #1521

johnspurlock opened this issue Jan 4, 2024 · 11 comments
Labels
feature request Request for Workers team to add a feature

Comments

@johnspurlock
Copy link

johnspurlock commented Jan 4, 2024

A common way for Cloudflare-unaware JS libraries to check if they are running in a web worker is: typeof importScripts == 'function'.

Once this passes, they assume other web-standard API like self.location is defined.

Perhaps workerd could use a blob url as the location here instead of leaving it undefined?

@jasnell jasnell added the feature request Request for Workers team to add a feature label Jan 4, 2024
@jasnell
Copy link
Member

jasnell commented Jan 4, 2024

Not sure there's really sufficient use case justification for this. How would you expect to use it? I'll note that none of the other primary non-browser runtimes (node.js, deno, bun, etc) implement self.location.

@johnspurlock
Copy link
Author

I ran into emscripten js boilerplate that does a "am I running in a web worker" check using typeof importScripts = 'function'. That actually passes on CF workers, but later on when it attempts to use self.location (unchecked, assuming it's there since importScripts was), it crashes. It just uses the .href for some default location probing logic.

I have a PR out for that on their side - honestly I was surprised importScripts was defined at all in CF workers. Does anyone use it? Removing importScripts would also fix this particular issue.

@hoodmane
Copy link
Contributor

For Emscripten feature detection I've been doing:

    // Force Emscripten to feature detect the way we want
    // They used to have an `environment` setting that did this but it has been
    // removed =(
    globalThis.window = {};       // makes ENVIRONMENT_IS_WEB    = true
    globalThis.importScripts = 1; // makes ENVIRONMENT_IS_WORKER = false
    const p = createEmscriptenModule(emscriptenSettings);
    delete globalThis.window;
    delete globalThis.importScripts;
    const emscriptenModule = await p;

@johnspurlock
Copy link
Author

Here's the PR over there in case anyone wants to +1: emscripten-core/emscripten#21010

For workerd tho, I'm still curious why importScripts exists - never seen a single cloudflare worker example where it is utilized

@hoodmane
Copy link
Contributor

hoodmane commented Jan 18, 2024

It's also not implemented:

jsg::Unimplemented importScripts(kj::String s) { return {}; };

Use Unimplemented as the return type of a method to mark the whole method unimplemented.
Attempts to use the API will throw an exception.

So removing it should be pretty harmless...

@jasnell
Copy link
Member

jasnell commented Jan 18, 2024

I'd be fine with removing importScripts(...) /cc @kentonv

jasnell added a commit that referenced this issue Jan 19, 2024
We have no plans to ever implement `importScripts()` and having
it defined but marked explicitly non-implemented is causing issues
with using certain third-party libraries in workers. So let's just
remove it entirely.

Refs: #1521
jasnell added a commit that referenced this issue Jan 21, 2024
We have no plans to ever implement `importScripts()` and having
it defined but marked explicitly non-implemented is causing issues
with using certain third-party libraries in workers. So let's just
remove it entirely.

Refs: #1521
@jasnell
Copy link
Member

jasnell commented Jan 23, 2024

Landed the PR that removes importScripts with a compatibility flag.

@jasnell
Copy link
Member

jasnell commented Jan 24, 2024

FWIW, the most reliable means of determining if code is running within the workers environment is to check the value of navigator.userAgent. Given that we've landed the PR removing importScripts under a compatibility flag, I'm going to go ahead and close this. The change likely would not hit production for another week or two.

@jasnell jasnell closed this as completed Jan 24, 2024
@johnspurlock
Copy link
Author

@jasnell, I'm getting ReferenceError: navigator is not defined when trying to access navigator.userAgent on Cloudflare Workers - am I missing something?

@mrbbot
Copy link
Contributor

mrbbot commented Jan 24, 2024

@johnspurlock I'm guessing you don't have the global_navigator compatibility flag enabled?

@johnspurlock
Copy link
Author

@mrbbot ah right, that new worker had no compatibility flag/date at all

{
  placement: {},
  compatibility_date: "",
  compatibility_flags: [],
  usage_model: "unbound",
  tags: [],
  tail_consumers: [],
  logpush: false,
  bindings: [
    ...
  ]
}

After redeploying with a compat date >= 2022-03-21, navigator.userAgent is defined, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature
Projects
None yet
Development

No branches or pull requests

4 participants