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 detection #457

Merged
merged 1 commit into from Sep 13, 2017
Merged

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented May 15, 2017

Currently in main script it is assumed that browser environment defines process object, that's not right:

screen shot 2017-05-15 at 10 55 00

I stumbled on it, when I tried to use debug in portable modules (ones that work on both server and client side) in a browser (via CJS bundler).

For time-being I'm relying on following workaround, but that's not quite beautiful.

This patch fixes it (and doesn't break proper Electron detection)

@coveralls
Copy link

coveralls commented May 15, 2017

Coverage Status

Coverage remained the same at 63.804% when pulling ba5b1da on medikoo:fix-env-resolution into 4a6c85c on visionmedia:master.

@TooTallNate
Copy link
Contributor

TooTallNate commented May 17, 2017

I think your client-side bundler is misbehaving. In the package.json file we have "browser": "./src/browser.js" which bundlers like browserify and webpack use as the entry point by default. The index.js file is intended for Node.js and Electron processes to use.

@medikoo
Copy link
Contributor Author

medikoo commented May 17, 2017

I think your client-side bundler is misbehaving. In the package.json file we have "browser": "./src/browser.js"

Not all bundlers support browser tag, and personally I prefer to rely on cleaner (imo) approach as feature detection directly in a module (it doesn't require any custom handling from a bundler)

I'm happy to see that you do feature detection in debug, still it doesn't seem right. Do you see any harm in change I proposed? Does it break anything?

@TooTallNate
Copy link
Contributor

I'm happy to see that you do feature detection in debug, still it doesn't seem right. Do you see any harm in change I proposed? Does it break anything?

It seems ok over first glance. I am skeptical though because we've learned through history of this module that tweaks to these parts of code can break edge cases. I suppose we could merge though and wait for any complaints to pop up. I still think you should use a better bundler though 😉

@medikoo
Copy link
Contributor Author

medikoo commented May 18, 2017

I still think you should use a better bundler though 😉

Actually in this bundler it's more a feature than a bug :) Idea is to work with portable CJS modules pure, least custom way (best if no customizations involved). Think of it as strict mode for CJS ;-)

I suppose we could merge though and wait for any complaints to pop up

I'll be grateful. The issue could appear only if some would run it in env where typeof process === "undefined" but still want the require('./node.js') to be loaded. As that contradicts each other (./node.js uses internally process object) change seems 100% safe.

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.

None yet

3 participants