separate "environment is node" from "environment has node"

[ericmandel]

... allowing hybrid environments (e.g. Electron.js) to use NODEFS

Thank you for submitting a pull request!

If this is your first PR, make sure to add yourself to AUTHORS.

[kripken]

Thanks for the PR!

This looks like it should work, and could be an interesting way to support electron, which I guess has elements of both node and web? I wonder if there are more such modes, though, and I'm definitely not the right person to ask. Might be worth looking through the bug tracker here to see other related requests, because I think I remember some (but not details).

[ericmandel]

FYI, my initial questions (and conclusions) leading up to this PR were in emscripten-discuss: https://groups.google.com/forum/#!topic/emscripten-discuss/2ncves7VbIw

I'll go through the bug tracker and point people to this PR, in the hope that we can get some help from experienced Electron developers.

[ericmandel]

An alternate proposal can be found here: #6889 where the environment is set to "node" explicitly:

var ENVIRONMENT_IS_NODE=typeof process==="object"&&process.browser !== true&&typeof require==="function" && ENVIRONMENT_IS_WORKER == false;

The difference between these two proposals is subtle but real: setting the environment to "node" in an electron app triggers many more Emscripten configuration options than does my PR. As a random example, in library_sockfs.js we see:

 // The node ws library API for specifying optional subprotocol is slightly different than the browser's.
 var opts = ENVIRONMENT_IS_NODE ? {'protocol': subProtocols.toString()} : subProtocols;

whereas my PR leaves the environment as "web" and only enables features that make use of node within the "web" environment.

I'm not sure which is "correct". Enabling node in an Electron web page is a run-time option (we plan to allow users to decide for themselves whether or not to mount the local file system) and my PR leads to similar Emscripten "web" initialization in both cases, whereas the alternate option either inits either as "web" or "node". We also envision possibly turning off user access to node functionality after the local file system is mounted under NODEFS, so in our use case we do have a "web" environment with some node functionality added.

But hopefully, there can be some discussion about this, based on other user cases.

[kripken]

Perhaps another option is ENVIRONMENT_HAS_REQUIRE - that is, to make the flag specific to the existence of that function. Then code can check for it specifically. That seems less ambiguous than "has node". On the other hand, is the idea that an environment like Electron is really node in some stronger sense?

[ericmandel]

Yes, the main Electron process (which is node + chromium libs) can display a web page in a separate renderer process, and one of the on-create options is to add full node integration into the rendered web page. So the web page is a hybrid of web and node. (I'd call it more web than node, because node has to be added to the web page explicitly, but others might differ because all of node is now available.)

One problem with making a single check, like HAS_REQUIRE, is that 'require' can be defined outside the node environment, e.g. if browserify is used. A two-check determination is less prone to false positives. In our case, 'require' is always defined because of a third-party module, even in a pure web page environment.

[ericmandel]

Hmmm ... upon reflection, your question about ENVIRONMENT_HAS_REQUIRE makes we wonder if I have failed to explain this PR clearly. So just in case (more detail is in the original discussion at https://groups.google.com/forum/#!topic/emscripten-discuss/2ncves7VbIw):

The aim is to be able to mount a local disk using NODEFS.mount() in an Electron web page in which the Node.js integration option has been enabled. Currently this fails because NODEFS.mount() checks the assertion that ENVIRONMENT_IS_NODE, which is false for Electron web pages.

The solution with the fewest side-effects seems to be separating ENVIRONMENT_IS_NODE from ENVIRONMENT_HAS_NODE and then changing the assert in NODEFS.mount() to check for ENVIRONMENT_HAS_NODE instead of ENVIRONMENT_IS_NODE. That is the essence of this PR.

[ericmandel]

@kripken Is there anything I can do to help this along?

[kripken]

I see, thanks! Ok, this sounds good to me. I've been waiting to hear if other people had feedback, but looks like there are no concerns, merging.

I think we should add some documentation for this. I'm not sure where, though. Probably in shell.js around where the new variable is defined?

[ericmandel]

Thanks, we really appreciate your acceptance of this PR, as the improvement can be staggering: loading a 500Mb 12K x 12K image without fetching into the heap takes 1s instead of 4s ... and obviously saves 500Mb of previous space.

I'm generally accused of over-documenting, but I'd recommend adding these lines in shell.js:

// A web environment like Electron.js can have Node enabled, so we must
// distinguish between Node-enabled environments and Node environments.
// This will allow the former to do things like mount NODEFS.
ENVIRONMENT_HAS_NODE = typeof process === 'object' && typeof require === 'function';

If you agree, can you do that or do you want me to make another PR??

[kripken]

@ericmandel yes, that sounds like a good addition to the docs, PR would be welcome!

[juanprietob]

Thank you for this PR. I am looking for this functionality and it will be great to have this soon.
I looked at the commit and I have a question.
Wouldn't the require fs and path

var fs = require("fs"); var NODEJS_PATH = require("path");

Need to be changed to something like

var fs = window.require('electron').remote.require("fs"); var NODEJS_PATH = window.require('electron').remote.require("path");
or
var fs = window.require('fs'); var NODEJS_PATH = window.require("path"); ?