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

Improve detection for Node JS #215

Merged
merged 1 commit into from
May 20, 2021
Merged

Improve detection for Node JS #215

merged 1 commit into from
May 20, 2021

Conversation

josephlr
Copy link
Member

@josephlr josephlr commented May 12, 2021

Fixes #214 (CC: @devdoshi can you verify that this fixes your issue)

This uses process.versions.node to see if we are in Node.js rather
than using global.self to check if we are in a Browser.

This change also makes some minor wasm_bindgen improvements:

  • Use js_sys::global() to get the global object
  • Bind the Node's require() as module.require
  • Improving error messages
  • Being more strict about JsValue types (i.e. check that they are objects rather than just making sure they are not undefined)

Signed-off-by: Joe Richey joerichey@google.com

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Looks good, but let's wait for confirmation from @devdoshi before merging.

BTW you have a typo in the PR title.

newpavlov added a commit that referenced this pull request May 14, 2021
@josephlr josephlr changed the title Imporve detection for Node JS Improve detection for Node JS May 14, 2021
This uses `process.versions.node` to see if we are in Node.js rather
than using `global.self` to check if we are in a Browser.

This change also makes some minor `wasm_bindgen` improvements:
  - Use `js_sys::global()` to get the global object
  - Bind the node require as `module.require`
  - Improving error messages

Signed-off-by: Joe Richey <joerichey@google.com>
devdoshi added a commit to opticdev/optic that referenced this pull request May 18, 2021
to reproduce:
$ OPTIC__BUILD_SKIP__UI=yes task postpull
$ yarn workspace @useoptic/ui-v2 test
@devdoshi
Copy link

Thanks @josephlr I'm checking right now via https://github.com/opticdev/optic/pull/786/checks?check_run_id=2611731376#step:6:25 but I still see the issues. Perhaps I'm not setting the dependency correctly? let me know if you can't see the logs but I believe you should be able to.

@devdoshi
Copy link

Now that I look at the issue with fresh eyes, I'm using Jest on a React UI project. It's set up to run in a jsdom environment. However, the jsdom environment does not provide an implementation for global.crypto. It appears I have to do that manually in the Jest setup, but I can definitely do that. So, I think the issue still remains that detection should be improved, but in my particular situation I think I should be making my jsdom environment include a crypto implementation, since jsdom is intended to make it look like it's in a browser, so your detection would probably continue to be fooled.

@newpavlov newpavlov merged commit 120a1d7 into master May 20, 2021
@newpavlov newpavlov deleted the detect branch May 20, 2021 08:01
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.

nodejs thinks it is in a browser environment
3 participants