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

[nw13] rethink the way we deal with Contexts #3107

Closed
rogerwang opened this issue Feb 19, 2015 · 28 comments
Closed

[nw13] rethink the way we deal with Contexts #3107

rogerwang opened this issue Feb 19, 2015 · 28 comments
Assignees

Comments

@rogerwang
Copy link
Member

From day 1 In NW we've been using a separate Node context from DOM context (or Window context). This was a design decision to support the following:

  • keep the namespace clean
  • keep objects alive when you navigate (DOM context will be destroyed at this point) and supports multi-page applications well
  • provide a way to share objects directly by reference between multiple windows.

But this has side effects like: comparison of types from different contexts; confusing libraries that supports Node.js already. There is a good survey and suggestion: #890 , and other issue like #1588, jsbin/jsbin#1150 (comment)

So the questions are: Should we keep window undefined in Node context? Should we keep a single context rather than Node/Window context split in NW?

What's your opinion from user's perspective? I'm eager to hear from you because this would be an important decision we're going to make when preparing for nw13, which will be a major step forward from previous versions.

@rogerwang rogerwang self-assigned this Feb 19, 2015
@rogerwang
Copy link
Member Author

Copying @Mithgol 's survey and suggestion from #890 here:

Different windows of a node-webkit's application have different JavaScript contexts; each window has its own global object and its own set of global constructors (such as Array or Object).

It's generally a good thing because when an object's prototype is replaced or augmented by a library (such as Prototype) or a simpler script, the analogous objects in other windows are unaffected nevertheless. Otherwise bugs could affect larger areas (several windows), malicious applications could access confidential data structures, etc.

And Node modules in node-webkit run in their own shared Node context for the same reason.

Such difference in contexts is generally benefitial, but sometimes it comes with a price. For example, a simple check such as value instanceof Array cannot determine if a variable's value belongs to one of the standard classes if it's passed from another context and had another object as its ancestor (see “Determining with absolute accuracy whether or not a JavaScript object is an array” for details).

Several issues of the past (#702, #716, #832) were caused by this difference of contexts and object inheritance.

That's why, for such special cases, I have to request an option for the window section of node-webkit's manifest (and thus for the options parameter in Window.open()).

That option should be false by default. If set to true, it should cause the corresponding window to run in the Node context (the same context that all Node code share).

The following Node.js globals should also become the globals (i.e. properties of the global object) in such window:

  • Standard object types: Array, Boolean, Date, Function, Number, Object, RegExp, String.
  • Typed array types: ArrayBuffer, DataView, Float32Array, Float64Array, Int16Array, Int32Array, Int8Array, Uint16Array, Uint32Array, Uint8Array.
  • Error types: Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError, URIError.

It is important to maintain [].constructor === Array and ({}).constructor === Object in that window, otherwise unobvious bugs would arise from using the most simple language constructs.

The name for such option could be nodeglobals or something similar.

The current workaround is using my nwglobal module. However, neither [].constructor === Array nor ({}).constructor === Object could be achieved with this workaround, so the real implementation of the requested feature should probably touch some deeper level of objects' structure of inheritance.

@johnlindquist
Copy link

We're going to see more and more libraries begin supporting both node/browser as more people rely on npm for client libs. They need to be able to determine which context they are run in.

For that reason only, I would suggest keeping window undefined in Node context.

With that being said, I don't fully understand all of the implications you listed above, so I'm not aware of all of the problems that may cause.

@Phoenixmatrix
Copy link

I've been running my apps completely in the node context to avoid having to worry about that. I manually set the window object and other globals, and for the most part, things just work.

IMO, the ability to have just 1 context is a huge benefit of nw.js... If I didn't want my use case above, I could just use AtomShell where the distinction is a lot more clear cut.

As in the suggestions above, making it an option would probably make everyone happy, but personally, I really like to think of nw.js as a normal node instance that just happen to be able to spawn windows (because of that, I'd also be pretty happy if I could do it just that way too... start in my node main and spawn my window from there, but that's a much bigger change, probably not for this discussion). 1 context (the node one) is very nice for that.

@cigitia
Copy link

cigitia commented Feb 19, 2015

Thanks for raising this issue; it's a very important and fundamental design question of NW.js. Thank you and everyone else also for NW.js itself, which is an incredible undertaking.

So the questions are: Should we keep window undefined in Node context? Should we keep a single context rather than Node/Window context split in NW?

What would the global window refer to in the Node context if the application has multiple windows that share their JavaScript contexts with Node (or, rather, io.js)? Or would only one window at a time be allowed to share its context with io.js?

@benjismith
Copy link

I've actually been pretty satisfied with the two-context system so far.

I have a simple (almost empty) index.html file, where I include a few browser-oriented javascript libraries (jquery, CKEditor, Leaflet, Stripe, and a few others). Each of these libraries defines globals (for example: $, CKEDITOR, L, and Stripe).

From the index.html page, I include a main.js script (in browser context) whose only function is to call require("MyApplication") and pass all the globals into node context. The rest of the application is developed completely within node context, and I've never had any problem with that setup. I can easily use libraries designed for node, since I'm primarily operating within node context, but I can also easily consume browser-based libraries by including them in my index.html and then passing their globals into my top-level node module.

@benjismith
Copy link

Actually, let me expand on that a little bit...

When I say that the rest of my application is in node context, I'm even including DOM manipulation (via jQuery), because I pass the jQuery object ($) from browser context into node context during the ready callback. The only code that executes in browser context is the code from third-party vendors (Leaflet, CKEDITOR, Stripe), and I handle all the events from those browser-context globals using callbacks executed in node context. It works like a charm.

If I wanted to use the window global from node context, I would pass it across the context boundary myself and treat it as a local variable in node context rather than as a global.

The only thing that feels awkward to me is that require is defined in browser context, which can cause confusion for scripts trying to determine their own context. It might be nicer if NWJS provided a single entry point where globals from browser context can be passed down into node context. Rather than hoisting require from node context up into browser context, it might be nicer if a global NWJS module provided its own bootstrapping entrypoint.

Maybe something like this:

<html>
<script src="jquery.min.js"/> <!-- defines global: $ -->
<script src="some-other-dependency.js"/> <!-- defines global: SomeOtherDependency -->
<script src="my-application.js"/> <!-- defines global: MyApplication -->
<script>
$( document ).ready(function() {
    NWJS.executeInNodeContext(function() {
        MyApplication.init($, SomeOtherDependency, window);
    });
});
</script>
</html>

Keeping node's require out of browser context, and defining a single entrypoint into node context, seems like it would both simplify and clarify the relationship between the two contexts.

@donaldpipowitch
Copy link

It can be difficult to get 6to5/babel (#3086) to work, if you want to use ES6 modules and translate them back to AMD modules which also need the require as a global variable. You need to rename nw's require before that. Using CommonJS instead of AMD results in using Node context instead of Window context. I also had problems with requiring modules like Rx which was added to global instead of window. All in all I had and have a lot of trouble because of both contexts. I

@biiiipy
Copy link

biiiipy commented Feb 19, 2015

I've had multiple issues with libraries, that try to detect node environment and so don't add global objects to window, which breaks apps if not manually patched.
I think Atom-Shell has a few good ideas, you should read this - https://github.com/atom/atom-shell/blob/master/docs/development/atom-shell-vs-node-webkit.md

@gpetrov
Copy link

gpetrov commented Feb 19, 2015

I will definitely go for the atomshell alike multi-context feature. So that all types come from the same global object and we don't have to deal with stupid bugs coming from instanceof comparisons like (#702, #716, #832) mentioned above.

@Mithgol
Copy link
Contributor

Mithgol commented Feb 19, 2015

Should we keep window undefined in Node context?

What problem does it solve if we make it defined?

(And it also can create problems with libraries that assume that typeof window === 'undefined' means Node.js support.)

Should we keep a single context rather than Node/Window context split in NW?

Two-context system is fine and helpful, but sometimes it also could be useful if a window could be explicitly specified to run in Node context (that's what #890 was about).

@donaldpipowitch
Copy link

I would like to behave nw.is that way: let me drop in any web app and it should work (e.g. also AMD based apps would work as require isn't by nw). Every unique feature of nw should be an opt-in and should be accessed with some single entry point.

@jonwwilkes
Copy link

tldr window by default in node context wouldn't make a whole lot of difference to me, and Atom Shell-style context separation would add complexity making certain designs very difficult.

I'm porting the GUI for Pure Data, a realtime audio programming app, from tcl/tk to nw.js. It is a multi-window application with editable diagrams in each window. (Plus a "main" window for error printout and audio controls.)

The core logic in C communicates with the GUI over localhost. I receive messages in a node context and manipulate the contents of the relevant window from there. (There are some window menus and a few other functions that live in the various window DOM contexts, but those usually just call some function in the node context.) If the "main" DOM were accessible automatically from the node context, it would only save me a single function call (i.e., setting a variable in the node context so that I can manipulate main window DOM from there).

I see some comments here praising the Atom Shell design. If nw.js went that route it would add an enormous amount of complexity to a multi-window design like mine. There are two reasons for this:
a) in Atom Shell the node context is not shared among multiple windows, iframes, etc. Consider that I have the following two lines in the script of a window that I open in atom shell:
var foo = require('foo');
foo.setBar(42); // sets 'var bar' in the node context to 42

Now let's say I open another window from my main.js, and the page in my new window contains this script:
var foo = require('foo');
var the_number = foo.getBar(); // fetch the value for 'var bar' in the node context
console.log("bar is " + the_number);

In Atom Shell, I cannot retrieve the value 42 because my node context isn't shared between the two windows. Because of this...

b) In Atom Shell, you use the ipc and/or remote module to send data among windows/iframes/etc. That's great for security, and it probably makes a lot of sense if you're porting web apps to the desktop which communicate through a REST API to a server somewhere. (Qt seems to have a similar type API for QWebView.) But for apps that are multi-window Atom Shell makes it expensive to incrementally design the GUI. (Prohibitively so in my case.) The ease with which you can get started in nw.js compared to Atom Shell is a testament to this.

@donaldpipowitch
Copy link

Added a new comment to #3086 (comment). I think this illustrates well how the current way of contexts can make debugging more difficult.

@slartibardfast
Copy link

I'd like to see the two context system remain in place by default but with the 'require' function (node context entry point) namespaced as something like nw.require (until one is within the node context).
Perhaps this namespace is be worthy of a configuration entry in the manifest.

At present, I using the inverse of this, with the requirejs optimizer being applied to
namespace a large set of AMD modules, as a bonus this setup allows the AMD
module's internal require object to functions as expected.

Agree that AMD modules working out of the box seems important.

@benjismith
Copy link

Agree with @slartibardfast... Once in node context, require should use the node mechanism, but in browser context, it should require namespacing to access. Apart from that, I'm very happy with the current system.

@donaldpipowitch
Copy link

If something like nw.require would be introduced it should also be considered, if we could introduce something like nw.gui as a global object.

@ghostoy
Copy link
Member

ghostoy commented Mar 13, 2015

Our product relies on uncaughtException in crash handler. In previous releases, uncaught exceptions or errors in node context is treated as a bug, and ignore errors in browser context. Once Node and Browser context mixed in a same context, it's hard to tell which context the error is from. It would be better to keep old behavior and provide a switch --mixed-context to turn on this new feature.

@szwacz
Copy link

szwacz commented Apr 22, 2015

Also remember that new ES6 modules syntax is on it's way to be native. Won't it be a problem for NW? Right now thanks to different way of loading scripts we know what is loaded to which context (require in node and <script> in browser). But what if both of those will use import keyword in not so distant future?

@Mithgol
Copy link
Contributor

Mithgol commented Apr 23, 2015

@szwacz Do you mean that the current require('async') and the future import "async" would work the same way in Node? Exactly the same way?

Because otherwise (i.e. if they are different) the community of Node.js, in order to abandon require(), would also have to abandon some (or even all) of the following useful features:

Theoretically such commitment to import is possible, but the current community seems far from it.

On the other hand, if the current require('async') and the future import "async" would work exactly the same way, then nw.js would pick one of them (e.g. import) to use a browser window's context and the other (e.g. require) to use the shared Node's context.

@rogerwang
Copy link
Member Author

Will introduce a mixed_context switch in package manifest in next 0.13.0-alpha2. When it is turned on, Node and DOM will use the same context. Thus there will be multiple Node.js contexts. And since the main entrance is the background page as in Chrome App, the DOM context of background page can be used to pass objects by reference between windows, serving the one of the purpose of Node context in nw12.

The default value for the mixed_context switch would be false.

@fxpoet
Copy link

fxpoet commented Jun 17, 2015

how to use 'nw.gui.window' in nw13?
I used the code as "nw.require('nw.gui')" but raised error.

@rogerwang
Copy link
Member Author

@fxpoet just use nw.Window . no need to call require('nw.gui') any more.

@rogerwang
Copy link
Member Author

v0.13.0-alpha2 is released with mixed context mode, To turn on the Mixed Context mode, add '--mixed-context' to the command line or set 'mixed-context' to true in manifest.

When it's off, it should have zero side effect on the normal mode.

https://groups.google.com/d/msg/nwjs-general/zeC6SedUSFY/BlumrYJeVHYJ

@AlbertSanIza
Copy link

@rogerwang Can you explain more your answer for @fxpoet... I can't make it work.

@fxpoet
Copy link

fxpoet commented Aug 21, 2015

I have solved this issue.
if (window.nw == undefined)
var win = require('nw.gui').Window.get(); //for version 12
else
var win = window.nw.Window.get(); // for version 13

@CxRes
Copy link

CxRes commented Sep 6, 2015

@Mithgol I am currently facing the same problem porting a jspm managed app. Because Systemjs provides a universal module loader for the browser, everything is being loaded in Webkit context. I had sometime back seen some serious discussion going on regarding AMD/CJS/ES6 module support at those fora (will link once I can find the page again).

For now I could do with a 'jailbreak' that will allow me to move over contexts arbitrarily while continuing to use require. Any Ideas?

@sean220
Copy link

sean220 commented Oct 13, 2015

Sharing objects directly by reference between multiple windows is very useful,it's make easy way to emit event between different windows ,I feel troubled and difficult to use the atom/electron's ipc implements。

@matthew-dean
Copy link

Agree with this:

I'd like to see the two context system remain in place by default but with the 'require' function (node context entry point) namespaced as something like nw.require (until one is within the node context).
Perhaps this namespace is be worthy of a configuration entry in the manifest.

Moving a generic "require" into the browser context can cause some unnecessary conflicts (as can exposing window to the Node context). That said, it's not been a major issue for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests