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

nodejs thinks it is in a browser environment #214

Closed
devdoshi opened this issue May 12, 2021 · 3 comments · Fixed by #215
Closed

nodejs thinks it is in a browser environment #214

devdoshi opened this issue May 12, 2021 · 3 comments · Fixed by #215
Assignees

Comments

@devdoshi
Copy link

I'm not sure if this is something particular to my setup but I was running into:
panicked at 'from_entropy failed: Web API self.crypto is unavailable', /Users/dev/.cargo/registry/src/github.com-1ecc6299db9ec823/rand_core-0.6.2/src/lib.rs:381:13

when using a module created via wasm-pack build --target nodejs ...

Based on my experimentation, it appears

// If `self` is defined then we're in a browser somehow (main window
global/global.self can exist in a nodejs environment so it follows the browser branch instead of the nodejs branch. I haven't set up a minimal reproduction but verified that delete global.self in my nodejs entry point resolved my issue. Thoughts?

@josephlr josephlr self-assigned this May 12, 2021
@josephlr
Copy link
Member

@devdoshi thanks for the bug report.

I was thinking of detecting Node.js presence like this: https://github.com/flexdinesh/browser-or-node/blob/master/src/index.js#L11-L13

Fundamentally, our detection can never be perfect (as users can always define weird things like global.self in Node or process.versions.node in the Browser) but we can make it less error prone.

@devdoshi
Copy link
Author

Thanks @josephlr - In my case I believe it was due to running the wasm module in Jest. I agree detection can never be perfect, but that logic you linked does look more robust. It would be nice to avoid the problem entirely though.

Do you think there would be a convenient way to have it be explicitly decided by the caller, so something can pass from e.g. wasm-pack (which knows the target) all the way through to here and let it be decided at build time? Perhaps an additional feature that gets tacked on in addition to js like js-browser | js-node?

I appreciate the rapid response!

@josephlr
Copy link
Member

josephlr commented May 13, 2021

I agree detection can never be perfect, but that logic you linked does look more robust.

Sounds good, if it's possible, see if #215 fixes your issue. Do you know if this would also require a backport to getrandom 0.1? My guess is yes, but I'd like to know on which version you are seeing this.

Do you think there would be a convenient way to have it be explicitly decided by the caller, so something can pass from e.g. wasm-pack (which knows the target) all the way through to here and let it be decided at build time?

I think adding this to the Rust+WASM tooling would be a good idea. Given that wasm-pack and wasm-bindgen know more about the environment in which they would be executing, they could provide some build time environment indication which would allow us to avoid needing dynamic environment detection. This could be done by:

  • Having wasm-pack or wasm-bindgen set an environment variable that we could parse in build.rs
  • A macro (or proc_macro) in wasm-bindgen that lets us have build-time environment detection.
  • An is_node()/is_browser()/is_web() function as part of wasm-bindgen or js_sys. This is less ideal because it would still be a runtime detection instead of a build-time detection.
  • Having separate Rust targets that indicate the environment, something like wasm32-unknown-js-node and wasm32-unknown-js-web instead of the wasm32-unknown-unknown target (which sort of sucks to work with). This would probably be ideal, but would be the most disruptive to the Rust+WASM ecosystem.

I'm not aware of anything like this existing today, but if you want to file an issue upstream, I'd be happy to advocate for a solution and explain getrandom's use case.

Perhaps an additional feature that gets tacked on in addition to js like js-browser | js-node?

We considered and rejected such an approach for getrandom 0.2, as it ends up being tedious/error-prone for users in practice, and doesn't solve the general problem. However, if one of the above solutions were implemented, we would likely use it to replace our existing detection code.

devdoshi added a commit to opticdev/optic that referenced this issue May 18, 2021
to reproduce:
$ OPTIC__BUILD_SKIP__UI=yes task postpull
$ yarn workspace @useoptic/ui-v2 test
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 a pull request may close this issue.

2 participants