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

Fix conditional check of options.sameOrigin #13

Merged
merged 1 commit into from
Oct 16, 2020

Conversation

eh-dub
Copy link
Contributor

@eh-dub eh-dub commented Oct 15, 2020

  • typeof always returns a truthy value ([MDN link](mdn instanceof vs typeof))

PR Type

  • Bugfix
  • Feature
  • Code style update (whitespace, formatting, missing semicolons, etc.)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other… Please describe:

Description

I changed the conditional check of options.sameOrigin in the constructor. The condition used to look at typeof options.sameOrigin which is always a truthy value (a string). Now it checks for the existence of the option directly.

The change is required to run this library in a nodejs environment because the window global variable does not exist.

How Has This Been Tested?

Before the code wouldn't run in a nodejs environment throwing the error window is undefined. Now the library works as advertised on nodejs environments.

Tested on Noder 14.9.0 on a Mac in an electron app.

Screenshots (if appropriate):

Does this PR introduce a breaking change?

  • Yes
  • No

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the contributing.md.

^ there is no contributing.md

- typeof always returns a truthy value
@beniz beniz requested a review from alx October 16, 2020 08:29
@alx alx merged commit caba5d6 into jolibrain:master Oct 16, 2020
@alx
Copy link
Collaborator

alx commented Oct 16, 2020

Hi @eh-dub

Thanks for the fix, new deepdetect-js@1.8.4 version has been pushed to npm: https://www.npmjs.com/package/deepdetect-js

Have a nice day,

Alex

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.

2 participants