Conversation
|
Looks good to me. This should also fix webpack I believe. @TooTallNate @tj thoughts? |
|
As a side note we really need to add test coverage. I hate having to rely, purely, on manual code review for this kind of stuff haha |
|
I was v careful because of this :) |
| * treat as a browser. | ||
| */ | ||
|
|
||
| if ((process || {}).type === 'renderer') { |
There was a problem hiding this comment.
In this case, I think process would always be defined, no?
There was a problem hiding this comment.
There are a few esoteric edge cases in Electron WebViews where it's been removed. These cases are very unlikely, but I wanted to just be extra paranoid
|
I know this isn't your issue, but I wonder why Electron doesn't respect the That said, this patch looks very nice and minimal. 👍 from me. |
| */ | ||
|
|
||
| if ((process || {}).type === 'renderer') { | ||
| module.exports = require('./browser'); |
There was a problem hiding this comment.
Maybe let's just add .js to the require calls to avoid extra stat() lookups.
The problem is that Electron is really its own thing, it is neither browser nor node (or really, both), and in general, I suspect that we'd be wrong more often (aka the library would exhibit incorrect / unusable behavior) by choosing browser rather than node. |
|
Looks good to me. Merging @TooTallNate @paulcbetts |
| } catch(e) {} | ||
|
|
||
| // If debug isn't set in LS, and we're in Electron, try to load $DEBUG | ||
| if ('env' in (process || {})) { |
There was a problem hiding this comment.
This broke component support. We've still got some old apps using it and https://github.com/component/cookie/blob/master/component.json#L8 broke the whole world :'(
...really need to get off of component, but still >_<
There was a problem hiding this comment.
This likely broke Bower support too, as I don't believe it shims process
4 participants
This PR attempts to resolve a few issues around
debugwhen using in Electron apps:Lots and lots of people use
debugin 3rd party node modules, and they use it viarequire. This means that we'll end up loadingnode.js. While this should work just fine, in Electron, this causes problems because in renderer processes (i.e. processes with a DOM),ttyand friends don't do The Thing You Expect (in fact, in older versions it used to crash!)When you attempt to fix this by loading
debug/browser.jsexplicitly, this might work, but if anyone before you has requireddebug,exports.logwill have been set to attyoutput, which means thatdebugis effectively disabledInstead, we're going to use Electron's
process.typefield to detect whether we're in a renderer process, and if so, we'll load the browser version. We'll also try to fetchprocess.env.DEBUGif LS isn't set to anything