ENVIRONMENT_HAS_NODE breaks with node-browser-libs

[lukebelliott]

Hello

45f74ea seems to break running in a browser with node-browser-libs, since the process global is defined.

A bit like #7514, this works for me:

ENVIRONMENT_HAS_NODE = typeof process === 'object' && !process.browser && typeof require === 'function';

Thanks

[kripken]

cc @ericmandel

[ericmandel]

@lukebelliott What actually breaks?

[ericmandel]

Yes, I think checking node-libs-browser process for the browser boolean can work to exclude that package, though I might be tempted to be a bit more specific and check its type:

ENVIRONMENT_HAS_NODE = typeof process === 'object' && process.browser !== true && typeof require === 'function';

The other thing we might do is check process.versions, which has been part of node since approx. v0.6.0 in 2011 or so:

ENVIRONMENT_HAS_NODE = typeof process === 'object' && typeof process.versions === 'object' && typeof process.versions.node === "string" && typeof require === 'function';

[lukebelliott]

For me it causes FS trouble:

Uncaught (in promise) Error: Cannot find module 'fs'

I just tried ENVIRONMENT_HAS_NODE = typeof process === 'object' && typeof process.versions === 'object' && typeof process.versions.node === "string" && typeof require === 'function'; and that works for me in browser, node and electron too.

Thanks

[ericmandel]

Thanks, I'll submit a PR with that updated feature test.

@kripken The code that broke is this:

  $NODEFS__postset: 'if (ENVIRONMENT_HAS_NODE) { var fs = require("fs"); var NODEJS_PATH = require("path"); NODEFS.staticInit(); }',

Aside from extending the feature test for Node, do you think it desirable to utilize 'try', e.g.:

  $NODEFS__postset: 'if (ENVIRONMENT_HAS_NODE) { try{ var fs = require("fs"); var NODEJS_PATH = require("path"); NODEFS.staticInit(); } catch(e) { ENVIRONMENT_HAS_NODE = false;  ENVIRONMENT_IS_NODE = false; } }',

Cause it's unlikely that a non-Node package would have process.versions.node defined, but not impossible ...

[ericmandel]

@kripken @lukebelliott I issued a PR containing the extended feature check for Node.

Wrt adding a try{} to $NODEFS_postset, I realize this could lead to an indeterminate state of affairs regarding the environment. So I'll leave the idea documented here, and we can revisit as needed.

[lukebelliott]

Cool, thanks for looking at it.

Would it be helpful to update #7559 with similar logic?

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.