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

shell.js: Handle if location object does not exist #17318

Closed
wants to merge 2 commits into from

Conversation

disjukr
Copy link

@disjukr disjukr commented Jun 26, 2022

When compiling with the options introduced in this comment, the ENVIRONMENT_IS_WORKER value is set to true.
In Deno, the value of self.location is undefined when the deno run command is executed without the --location CLI argument.
In the scenario of creating the Deno library, it is difficult to tell the library user to input the --location CLI argument, so I modified it to handle fallback at this level.

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

In general this sounds good. Please take a look at the CI errors though, and let us know if you can't figure them out.

One possibility is that ?. notation may not be supported in one of our tools (I don't think we use it anywhere atm). But that's just a guess.

When compiling with the options introduced in [this comment](emscripten-core#13190 (comment)), the `ENVIRONMENT_IS_WORKER` value is set to `true`.
In Deno, the value of `self.location` is `undefined` when the `deno run` command is executed without the `--location` CLI argument.
In the scenario of creating the Deno library, it is difficult to tell the library user to input the `--location` CLI argument, so I modified it to handle fallback at this level.
@kleisauke
Copy link
Collaborator

Regarding the ?. notation, this is being tracked in issue #15827.

src/shell.js Outdated Show resolved Hide resolved
src/shell.js Outdated Show resolved Hide resolved
Co-authored-by: Alon Zakai <alonzakai@gmail.com>
@kripken
Copy link
Member

kripken commented Jun 29, 2022

I looked into this a little more now. It seems like node also does not define location (or self for that matter). We work around that here:

emscripten/src/worker.js

Lines 31 to 37 in 1056be2

Object.assign(global, {
self: global,
require: require,
Module: Module,
location: {
href: __filename
},

@disjukr is the problem with Deno also just when using pthreads? If so, then I think working around it in worker.js like we do for node might make sense.

@disjukr
Copy link
Author

disjukr commented Jul 17, 2022

@kripken

@disjukr is the problem with Deno also just when using pthreads? If so, then I think working around it in worker.js like we do for node might make sense.

In my situation, when the -pthread option is attached, an error occurs saying that it cannot be used with the --shared-memory option, and the build fails.

wasm-ld: error: --shared-memory is disallowed by enc/webp_enc.o because it was not compiled with 'atomics' or 'bulk-memory' features.

I'm not familiar with c/cpp so I can't narrow down the reproduction path properly, but when I built a very simple pthread example, the ENVIRONMENT_IS_WORKER variable is set to false, so the branch accessing self.location.href does not run.

@disjukr
Copy link
Author

disjukr commented Jul 17, 2022

Oh sorry. I noticed that I accidentally included the -s ENVIRONMENT=$(ENVIRONMENT) (and that expanded as -s ENVIRONMENT=worker).
I tried to build without this option and the branch that access self.location.href never runs.

@disjukr disjukr closed this Jul 17, 2022
@disjukr disjukr deleted the patch-1 branch July 17, 2022 03:03
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

Successfully merging this pull request may close these issues.

3 participants