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 browser environment detection #328

Closed
wants to merge 1 commit into from
Closed

Fix browser environment detection #328

wants to merge 1 commit into from

Conversation

jahed
Copy link

@jahed jahed commented Nov 11, 2016

The Electron detection has broken Browser detection. Since browsers don't have a process variable, they end up include node.js and all the fs, net, etc. modules with it.

This should fix it, however since there aren't any tests (which would've caught this breakage), I can't be 100%.

jahed referenced this pull request Nov 11, 2016
This is also more readable in my opinion.
@TooTallNate
Copy link
Contributor

What module loader are you using? It should pick up the browser: "./browser.js" entry in the package.json file and use that as the entry point for browsers.

@jahed
Copy link
Author

jahed commented Nov 11, 2016

I'm using Webpack. Didn't know about the main/browser branching in npm, not sure if Webpack uses it. It's definitely going through the index.js route at the moment.

@TooTallNate
Copy link
Contributor

It definitely does, by default at least. You might have a strange configuration...

@jahed
Copy link
Author

jahed commented Nov 11, 2016

Hmm, I'll look into it further. The dependency on debug is coming from a nest of dependencies so it could be something there. Strange thing is, it's been working fine until yesterday and our config is very plain.

I'll close the PR for now.

@jahed jahed closed this Nov 11, 2016
@TooTallNate
Copy link
Contributor

The funny part to me is that your PR touches the latest commit in master, which hasn't even been published to npm yet. Are you pulling from master for some reason?

@jahed
Copy link
Author

jahed commented Nov 11, 2016

The change wasn't specifically for the latest commit. More for the branching logic in the file.

The dependency on debug comes from jsonp which recently had a change from a strict dependency on 2.1.3 to minor (I did a clean npm install yesterday so this change was picked up now). jsonp is relied on by auth0.js and so on.

Looking deeper into it, the package.json at that point also used the main/browser split. Even though this PR fixed the issue for me by directing Webpack to the browser.js via index.js, it's probably something else causing Webpack to go through the main route instead of browser in the first place.

So the issue can be from any number of things, I'll probably need to investigate more on my end if no one else is reporting similar issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants