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: update is-browser.ts to account undefined navigator #1868

Merged
merged 2 commits into from
May 17, 2024

Conversation

cmoniz-ocean
Copy link
Contributor

@cmoniz-ocean cmoniz-ocean commented May 17, 2024

Fixes error

ReferenceError: navigator is not defined
    at isStandardBrowserEnv (node_modules/mqtt/src/lib/is-browser.ts:7:4)

(error is thrown in electron 12.2.3 and electron 30.0.6)

6a03d29#commitcomment-142114121

Fixes electron 12.2.3, which doesn't have navigator defined

mqttjs@6a03d29#commitcomment-142114121
@cmoniz-ocean cmoniz-ocean changed the title Update is-browser.ts to account for undefined navigator fix: Update is-browser.ts to account for undefined navigator May 17, 2024
@cmoniz-ocean cmoniz-ocean changed the title fix: Update is-browser.ts to account for undefined navigator fix: Update is-browser.ts to account for undefined navigator May 17, 2024
cmoniz-ocean referenced this pull request May 17, 2024
* fix(electron): detect electron env

* fix(electron): cleanup code

* fix: fixed wrong operator

* fix(electron): improved code and add some comments

* Update src/lib/is-browser.ts

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>

* fix: typo and lint

---------

Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
@robertsLando robertsLando changed the title fix: Update is-browser.ts to account for undefined navigator fix: update is-browser.ts to account for undefined navigator May 17, 2024
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

Did you verified this is actually working on Electron?

@@ -4,6 +4,7 @@ const isStandardBrowserEnv = () => {
// Is the process an electron application
// check if we are in electron `renderer`
const electronRenderCheck =
(typeof navigator !== "undefined") &&
navigator?.userAgent?.toLowerCase().indexOf(' electron/') > -1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
navigator?.userAgent?.toLowerCase().indexOf(' electron/') > -1
navigator.userAgent?.toLowerCase().indexOf(' electron/') > -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertsLando I've run npm run lint-fix and made the above changes #1868

Additionally, this has been npm run builded, npm packed, and then npm installed to my local electron 30 project and looks like it's working.

@robertsLando robertsLando changed the title fix: update is-browser.ts to account for undefined navigator fix: update is-browser.ts to account undefined navigator May 17, 2024
@robertsLando
Copy link
Member

Also pleasew fix lint issues with npm run lint-fix

@axi92
Copy link
Contributor

axi92 commented May 17, 2024

I am happy to verify the changes later as well!

@robertsLando robertsLando merged commit 0111a7a into mqttjs:main May 17, 2024
3 of 4 checks passed
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.

None yet

4 participants