Don't detect nw.js as node.js #10228
Conversation
|
Here is the nw.js documentation that explains |
There was a problem hiding this comment.
Once this is done, please squash the commits into one; refer to https://github.com/mozilla/pdf.js/wiki/Squashing-Commits for how to do that easily. Thank you!
src/shared/is_node.js
Outdated
| return typeof process === 'object' && process + '' === '[object process]'; | ||
| return typeof process === 'object' && | ||
| process + '' === '[object process]' && | ||
| !process.versions['nw']; |
There was a problem hiding this comment.
Can you change it to something like this for readability and explanation of why we exclude NW.js?
// Exclude NW.js since, even though it's based on Node.js,
// it works like a browser.
return typeof process === 'object' &&
process + '' === '[object process]' &&
!process.versions['nw'];
|
Done, thanks for the doc. |
src/shared/is_node.js
Outdated
| return typeof process === 'object' && process + '' === '[object process]'; | ||
| // NW.js is a browser context, but copies some node objects. | ||
| // For more information see the 'Separate Context Mode' doc: | ||
| // http://docs.nwjs.io/en/latest/ |
There was a problem hiding this comment.
Why not link directly to the relevant page instead, to save people from having to search around for the referenced content?
Please just change the entire comment to something like this instead (note that URLs in comments are exempt from the general line-length limit):
// NW.js is a browser context, but copies some Node.js objects; see
// http://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#separate-context-modeThere was a problem hiding this comment.
NW.js doc evolves so I prefered to not give a link that could be broken later, then harder to resolve than domain name + section name.
nw.js is chrome plus nodejs. It will succeed everywhere chrome succeeds, but fail in many cases where nodejs succeeds (see issue 9071). So it's safer to consider it as a browser context rather than a nodejs context. Make travis happy again CS Readability + Explanation The relevant portion of the NW.js documentation: http://docs.nwjs.io/en/latest/For%20Users/Advanced/JavaScript%20Contexts%20in%20NW.js/#access-nodejs-and-nwjs-api-in-browser-context Added full link to relevant doc.
|
Done and squashed. |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/2e3f2b12ead2771/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/8cddad450a190a6/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/2e3f2b12ead2771/output.txt Total script time: 19.13 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/8cddad450a190a6/output.txt Total script time: 24.77 mins
|
|
Let's do this. Thank you! |
nw.js is chrome plus nodejs.
It will succeed everywhere chrome succeeds, but fail in many cases where nodejs succeeds (see issue 9071).
So it's safer to consider it as a browser context rather than a nodejs context.
Fixes #9071.