whatwg / loader

Loader Standard
https://whatwg.github.io/loader/
Creative Commons Zero v1.0 Universal
609 stars 45 forks source link

System.install to update module export references #30

Closed guybedford closed 9 years ago

guybedford commented 9 years ago

If I have a module, the standard scenario with hot-reloading is that I want to alter the dependency and then have that new reference automatically pushed out to all modules that may have been loading it.

Because modules naturally share this export reference, if System.install can not just set the module in the registry, but update the export references in dependent modules so that it pushes out to all modules already using the old dependency, it would be a really nice use case. It's exactly what assignment to an export variable does already. Code not based on side-effects (ala React) would effectively get natural hot-reloading support for modules (components).

matthewp commented 9 years ago

I like the idea, is this not possible to do in userland?

caridy commented 9 years ago

I do sympathize with this idea, but I'm concern about it, which I will express in another post, for now, I will like to clarify one piece, we should not use React as the example since this is an anti-pattern in react where objects are not observables, and instead you modify states thru helper methods. That's the reason why they decided to remove the ability from a parent component to access a child component and update one of its members, because there is no way to track the changes without observing every component. In other words, we should not rely on the live bindings mechanism to trigger function calls in consumer module, not without some sort of observable pattern in place to detect those changes.

guybedford commented 9 years ago

Even if we forget about React - a lot of modular code is stateless. Hot-swapping within stateless modules is given completely for free with this approach.

Yes hot-swapping within stateful use cases relies on hooks etc in user code, but it's ultimately a completely separate scenario, and the custom tooling required here can build on top of the stateless binding approach.

Consider also the converse - if we do not do something like this, then System.uninstall is effectively leaving the module bindings where they remain. Since modules build on top of the concepts of shared bindings, I'd argue pushing out undefined for its former named exports when a module is uninstalled makes complete sense here.

If the loader doesn't implement anything like this, hot swapping becomes near impossible though.

guybedford commented 9 years ago

To clarify the benefits:

In terms of managing a hooking system for stateful changes, I guess it would be through some kind of global event registration (implemented by the custom loader implementation, not spec of course):

import {key} from this module;
import {fn} from './dep';
var depKey = System.resolve('./dep', key);
System.on('moduleupdate', function(key, module) {
  if (key == depKey) {
    fn = module.fn;
  }
});

export function update() {
  return fn();
}
caridy commented 9 years ago

We are getting here into a very very dangerous territory. thinks about this:

var foo = {
    bar: {
        something: function () {}
    }
};

var x = foo.bar;
delete foo.bar;

Essentially, the fact that we have install/uninstall has nothing to do with setting values to undefined, otherwise you can't really predict what's going to happen, what will break once you uninstall the module.

I don't think the loader should mess with any of this. If we get to consensus that altering the bindings of the module is a good feature, it belongs to the module rather then loader.

guybedford commented 9 years ago

@caridy if I have:

a.js

export var obj = { some: 'value' };
export function clear() {
  obj = undefined;
}

b.js

import {obj,clear} from './a.js';
var x = obj;
clear();

The above will not set x to undefined. So I'm not sure I see where the danger is? As I see it, uninstalling a module without doing something like this is just as dangerous as you will never know what traces you might have left behind.

matthewp commented 9 years ago

If there were a way to get a module's dependencies (something requested in another issue) you could simply reinstall any dependent modules to update their bindings. (but then what happens to that module's bindings...)

dherman commented 9 years ago

The module system was designed from the beginning to be early-bound: imports are resolved once-and-for-all and linking is hard-wired, just like lexical bindings. The loader system is itself dynamic, so you can certainly modify the global table. But we should not blur the two. If you want to build hot-reloading frameworks, I'd suggest experimenting with them on top of the configurability of the loader.

guybedford commented 9 years ago

I'd argue not doing something like this is breaking the assumptions of the module system. The module system bindings are based on the principle that if a module imports an export from another module, then the binding of that import is the same as the binding from the export scope. But if we arbitrarily allow install to replace existing modules in the tree, that have already been imported, without considering what this means for the bindings, we're breaking this assumption in the registry.

caridy commented 9 years ago

Are we in agreement here? can we close this?

guybedford commented 9 years ago

Ok, here's my problem - I can add maybe just 20 lines of code to the polyfill work to get hot reloading, which immediately allows the polyfill to compete directly with webpack on par for features.

But the issue is that if this feature is off-spec, I then need to look at how to build the feature in a way that is specification-compatible. Which means getting dependency traces, which aren't even possible in the spec. Thus the feature is not possible to implement otherwise without a massive amount of tooling.

That is, the polyfill would no longer be a polyfill. So we either go against the spec in order to match webpack for features (very frustrating when it is only 20 lines of code), or we can try and get simple install bindings for hot reloading in the spec. That is why I'm trying to justify the feature here as I do think hot reloading is a very important feature to users, and they seem to naturally expect it now that webpack has set the bar.

arv commented 9 years ago

As an implementer (V8 not Traceur) I'm highly opposed to hot swapping. The way identifier expressions work is that they point directly to a point in memory (or on the stack in some cases). Hot swapping would mean that the new code would have to reuse the same memory locations.

Also, what happens when the new module no longer has a binding for something?

@guybedford You can achieve most of this in user code. Lets play around with that and see if that solves your use cases before we make the whole system a lot more complicated.

guybedford commented 9 years ago

When you update an exported binding from within the exporting module in ES6 currently, it reassigns the binding anyway:

export var p = 5;
p = 4;

So surely this is the same mechanism? The same would happen here if there was no export for a given slot as if you later assigned an export to undefined:

export var p = 5;
p = undefined;

Does that sound right?

The best performance would come from the loader spec itself, because we can replace just the modules that changed pushing through the bindings (without having to reload every parent module again). As non-side-effect modules become more and more standard, this is a potentially powerful and quick development workflow. The only user-code alternative to that exact same mechanism is to have the feature in the polyfill under a flag.

arv commented 9 years ago

@guybedford The problem is that when you parse the second module you would need to make sure you use the same memory location as the previous time.

Instead, you can do something along the lines of (in user code):

// hotswapper

import * as initial from './initial.js';

export let x = initial.x;
export let y = initial.y;
...

export function swap(newUrl) {
  System.importModule(newUrl).then(m => {  // this needs to use local module import, TBD
    x = m.x;
    y = m.y;
   ...
  });
}
guybedford commented 9 years ago

@arv I'm not sure I understand what you mean here. With ES6 modules if I do the following:

export var p = 5;
p = { new: 'binding' };

Surely we've just updated the module binding to reference a new location in memory?

arv commented 9 years ago

@guybedford The binding memory location does not change. What that points at will have changed though.

guybedford commented 9 years ago

So the proposal is simply to use this same mechanic to update the import references in the dependent modules that have already loaded a module when installing an update to it. This way we just refresh the changed module, and get an incredibly quick dev workflow.

guybedford commented 9 years ago

@arv I've updated the title - let me know if the terminology is still incorrect.

arv commented 9 years ago

Refreshing and replacing a module is the same thing from a VM's perspective.

I still don't understand how this would work in practice without having to deopt the whole world. Just imaging that the VM might have inlined a const binding (which it is allowed to do). Now with hot swapping an exported const binding might change its value.

johnjbarton commented 9 years ago

Are dynamic updates possible using an alternative non-VM embedded loader in the presence of an optimizing VM embedded loader?

caridy commented 9 years ago

@guybedford lets step back a little bit here... the way I see it, this use case is essentially an artifact for development where you want to fetch a single module (changing module) and update it in the runtime. That alone is a very very edge case, and considering the whole bundling/folding process in place in most cases, this is even more rare.

But let's assume for a moment that we think it is a must-have. Looking at what is possible today, you will encounter two cases with CJS:

var Foo = require('./something').Foo;
...
module.exports = function () {
    return new Foo();
}

in the example above, there is no way for browserify/webpack, or any other tool out there to update the value of Foo when ./something module changes, unless the revamp the dep tree of that module.

To be practical, in order to accommodate that, you will have to do with:

...
module.exports = function () {
    var Foo = require('./something').Foo;
    return new Foo();
}

this will allow the tooling to update the module entry, invalidate the cached entry, so you get a new instance of the module in the next call, the thing is, most people will never sacrifice the perf of my code to satisfice a development practice, especially if you know the implications of that.

I truly believe we are chasing ghost here.

matthewp commented 9 years ago

@caridy

unless the revamp the dep tree of that module

What do you mean by this?

caridy commented 9 years ago

It means that not only ./something has to be "refreshed" when it changes, but all its dependents too.

matthewp commented 9 years ago

Not if the dependents imported the entire module, no? But if we wanted to support this, what is the issue with reloading the dependents?

caridy commented 9 years ago

Let me clarify, in order to match webpack/browserify/others hotswap today, we just need to have the tooling to revamp the whole tree or a branch affected by the changing module. We don't really need any low level feature for that. I'm also saying that hotswap for a single module does NOT work today in any of those tools without doing the revamp of the tree or a branch anyways.

matthewp commented 9 years ago

That sounds reasonable, but we cannot do this in user code because as @guybedford mentioned we do not have the ability to get the loader's internal graph.

guybedford commented 9 years ago

@arv this most likely would deoptimize everything yes. But surely only at the point of installing? The thing is exported module bindings can change at any time surely anyway (ala circular reference execution)? Unless I'm mistaken here, in which case that would be incredibly valuable to know.

@caridy yes matching behaviour involves replacing all dependents. As @matthewp says we can't enable this feature either without knowing dependencies from the loader.

Currently install leaves dangling references to a replaced module, I'm just suggesting we alter this scenario to update them, which doesn't take anything away? Then in a scenario like React though this works out incredibly elegantly as component classes are re-instantiated on each render cycle making for a really nice dev workflow.

Yes it's an edge edge case, but I'm not sure I agree with the edge edge case argument - development gains can't be over-valued.

caridy commented 9 years ago

@guybedford the tooling I'm referencing to can achieve the same by analyzing the changing file (on the watch), computing the dep graph (just like it will do it when bundling/folding is required) and it can signal the client to uninstall a batch of modules affected by the offender module (which will always affects the top level import call), meaning you will probably call loader.import() again on the top level module to refresh all those modules.

And yes, this is a very very edge case. Even the example you mentioned about react is not realistic or worth it, otherwise facebook will have that implemented a long time ago considering that they have apps with 1000+ components. /cc @jeffmo

matthewp commented 9 years ago

@caridy That is a lot of tooling you're asking for. We should make this work better in the browser (this is whatwg/loader after all, not nodejs/loader). Bundling (not sure what you mean by folding) is a legacy need.

A better, browser-based, workflow would only require a process that watches for file changes and send a message to the browser of which module changed. Everything else should be doable from a custom loader.

caridy commented 9 years ago

Everything else should be doable from a custom loader.

That's a moon shot right there. I will strongly oppose to adding anything that can be implemented in user land with the right tooling, especially if this is not a common use-case.

Let me ask you a question: who is using such feature today? I know no one relying on such thing in their development workflow. The way I see it, people try it, it is a cool feature, but they eventually drop it due to the complexity and constrains it adds, similar to what I explained above with the CJS example.

caridy commented 9 years ago

@caridy That is a lot of tooling you're asking for.

@matthewp that's the same amount of tooling you will need today to implement the hotswap in cjs :), or maybe I'm missing something here. jejeje

matthewp commented 9 years ago

What is a moon shot? All we need is:

  1. A way to get the loader's dependency graph (this should be provided anyways, it's useful in a lot of scenarios).

And that's it. Otherwise this can be implemented in userland very easily.

Let me ask you a question: who is using such feature today?

Hot swapping is very popular in the lisp world. Otherwise it's become popular for Webpack users because this is a feature of that bundler. Here's a popular project with over 1000 stars that does this.

I don't know about giving it up because of constraints because I use a Loader loader and Loader doesn't allow this right now :) Let's fix that.

caridy commented 9 years ago

@matthewp that is extremely misleading. there is absolutely nothing in that project that depends on a loader, it is all magic implemented by tooling the modules before delivering them to the browser. Swapping a module is just this:

    (function() {
        "use strict";
        var foo = "2";
        exports.foo = foo; /* REACT HOT LOADER */
    })();
    if (true) {
        (function() {
            module.hot.dispose(function(data) { // @caridy: this was instrumented in the previous turn
                data.makeHot = module.makeHot;
            });
            if (module.exports && module.makeHot) {
                var makeExportsHot = __webpack_require__(7), // @caridy: this is computed by server
                    foundReactClasses = false;
                if (makeExportsHot(module, __webpack_require__(12))) { // @caridy:this is computed by server
                    foundReactClasses = true;
                }
                var shouldAcceptModule = true && foundReactClasses;
                if (shouldAcceptModule) {
                    module.hot.accept(function(err) { // @caridy: this was instrumented in the previous turn
                        if (err) {
                            console.error("Cannot not apply hot update to " + "foo.js" + ": " + err.message);
                        }
                    });
                }
            }
        })();
    } /* WEBPACK VAR INJECTION */

essentially, it is just instrumenting the code that you deliver to the browser to wrap everything so you can control what to do when a particular module is swapped.

as for the live example (http://gaearon.github.io/react-hot-loader/), we should not use that as a reference since it is doing things manually to eval and re-render the a single blob that does everything.

again, there is nothing here. you don't need to tap into the loader to inspect or control the graph, that's absolutely not needed for this, nor for any other feature that we have encounter so far.

matthewp commented 9 years ago

there is absolutely nothing in that project that depends on a loader, it is all magic implemented by tooling the modules before delivering them to the browser.

I know it doesn't depend on a loader; that's the point. The Loader spec needs to compete with the likes of Webpack because if it doesn't people won't use it. I disagree with your point that if it can be done with (non-browser based) tooling then we don't need it. All Loader functionality can be done (and is being done today) without a browser Loader but I think we all agree that this spec is a good idea.

I'd love to implement hot reloading with the spec we have today, but to do so I'd have to load something like esprima and parse every source in translate. This is not ideal though. There was another issue where we discussed losing deps as part of instantiate, there was another use-case there for getting the dependency graph.

guybedford commented 9 years ago

I think we can all agree hot reloading is a valuable use case scenario for users, and as such it is worth considering how the loader fits into this picture.

The fact that there is a System.install pretty much enables the entire use case, which is amazing.

While dependency analysis would be useful for a lot of scenarios, a server counterpart is always needed for hot reloading so it is not critical.

All I'm asking is that we consider this side case of the System.install behaviour as it might enable a more performant hot reloading workflow in a subset of scenarios that could become more dominant in future for large module trees. The arguments against have all centered on updating the exports being unorthodox, which makes me worry there's still some misunderstanding that this isn't the way module exports already behave. It's a really small change as it's already the way things work, unless I'm mistaken?

caridy commented 9 years ago

@matthewp who say we want to beat webpack? those tools are not going to disappear, they will continue to exist to pack/optimize our code, and I can see them providing "folding" capabilities for modules as part of that optimization, but that's a separate discussion.

@guybedford fair enough. can you re-state the feature that you want to be considered? jejeje, too much noise I guess...

guybedford commented 9 years ago

Ok, let me start at the beginning one more time, and try to be succinct.

Currently updates push out already:

stuff.js

export var stuff = 'version 1';

app.js

import {stuff} from './stuff.js';
export function render() {
  console.log(stuff); // super advanced virtual rendering mechanism
}

Now inside of stuff.js, if I reassign the stuff variable, I also update the binding inside of app.js. So we effectively already have the system set up to push out an update to the binding.

My suggestion is that System.install('stuff.js', Module({ stuff: 'version 2' })) also pushes out the new binding in the exact same way modules already work, so that we get some limited updates enabling a high-performance minimal hotswapping subset for code that might work with it, without having to re-run all scripts that import from stuff (which can certainly be enabled too through tooling like Webpack).

Currently, the way System.install is written, stuff stays as version 1 inside of app.js, so it could even be seen as being memory-beneficial to update the binding.

If I'm wrong about this being natural to how the module bindings already work, then please let me know.

If I haven't thought this through, and this isn't actually useful to React at all, let me know too.

arv commented 9 years ago

Now inside of stuff.js, if I reassign the stuff variable, I also update the binding inside of app.js. So we effectively already have the system set up to push out an update to the binding.

There is only one binding. There is no pushing or updating of a binding going on.

My suggestion is that System.install('stuff.js', Module({ stuff: 'version 2' })) also pushes out the new binding in the exact same way modules already work

What it needs to do is to find the old module and find the bindings it has and reassign them. However, you cannot reassign them if they are const bindings.

If I'm wrong about this being natural to how the module bindings already work, then please let me know.

This is no more natural than changing the values of variables inside a function/block from outside the block.

Or even more analogous, replace a function with another but reuse the variable bindings inside it.

memory-beneficial

Don't forget that closures that captured bindings in the old module will keep things alive.

I'm not saying that this is impossible to achieve, I'm just saying that it is going to be very complex and lead to a lot of potential issues.

guybedford commented 9 years ago

@arv ok there may be a misunderstanding over the bindings which would be good to clarify.

I have been under the impression that the following code would work:

dynamic.js

export function update() {
  q = 10;
}
export var q = 5;

app.js

import {q, update} from 'dynamic';
assert(q === 5);
update();
assert(q === 10);

If this is not the case, then yes my entire suggestion falls over. It would be good to discuss the edge cases if so, in terms of circular references again for transpilation to ES5, as this is the behaviour we've implemented.

guybedford commented 9 years ago

@arv the const argument does sound like an issue. Are function exports constants?

caridy commented 9 years ago

that example works just fine, but @arv is talking about consts.

in anycase, this is a no go for me for the following reasons:

guybedford commented 9 years ago

Thanks everyone for listening to see if a highly-optimized dynamic hot-reloading scenario was within grasp. The export const issue argument nails this the most for me in terms of what it means for the engine.

In short, the way to handle hot reloading is to:

All of the above is perfectly possible within the module loader.

As a side note, I do agree with @matthewp on the point that exposing module dependency arrays is a useful low-level API to have, and have mentioned this before as well myself.

@caridy closing, but if you have a change of heart let me know :)

caridy commented 9 years ago

:+1:

dchambers commented 9 years ago

As a side note, I do agree with @matthewp on the point that exposing module dependency arrays is a useful low-level API to have, and have mentioned this before as well myself.

Can I add another vote for this feature too, but for a different use case. Unit testing of code often requires that certain sub-systems (or collaborators) are mocked, and although it's often possible to design modules so that their dependencies are injected in (e.g. via constructors), this is not always easy to do, particularly when third-party code is involved.

Libraries like proxyquire solve this for NPM libraries by using require() as a seam for performing IoC at will throughout the code-base. In the same way, something similar could be done for the ES6 module loader. Unlike hot-swapping however, unit testing is normally done entirely on the client-side, and so without access to the module dependency arrays on the client side then every module would need to be re-loaded in every test where mocking was performed.

Now, presumably, this might also cause issues if there were stateful modules that the test author wasn't responsible for, and so couldn't change? Perhaps it might even cause havoc with the test runner or testing framework itself? In any case, it would seem both safer and more performant to only replace the modules that actually needed to be replaced based on what was being mocked.

@caridy, does this add any weight to the argument?

dchambers commented 9 years ago

Oh, to give you an idea of what these mocking tests might actually look like, here is how I'm envisioning things maybe working:

'use strict';

import MockableSystem from 'MockableSystem';

define('Vending Machine', function() {
    beforeEach(function() {
        MockableSystem.install();
    });

    afterEach(function() {
        MockableSystem.uninstall();
    });

    it('always vends chocolate if that is all there is', function() {
        MockableSystem.redefine('vendable', 'ChocVendable');
        let vendingMachine = System.import('VendingMachine').create();

        expect(vendingMachine.vend()).toBe('choc');
    });

    it('always vends chocolate if that is all there is (variant)', function() {
        MockableSystem.redefine('vendable', {default:function() {
            return 'choc';
        }});
        let vendingMachine = System.import('VendingMachine').create();

        expect(vendingMachine.vend()).toBe('choc');
    });
});
capaj commented 8 years ago

I just stumbled on this thread this evening and it ruined my evening. It is really a shame that the export const is the only culprit which made @guybedford's suggestion not being possible. I recently implemented a hot reloading solution on top of JSPM and I would love to get rid of traversing the dep tree and reloading it on a single file change.

Don't forget that closures that captured bindings in the old module will keep things alive.

@arv that is actually not a problem with frameworks like React/Mithrill.js plus lots of others which use virtual DOM/similar.

this use case is essentially an artifact for development where you want to fetch a single module (changing module) and update it in the runtime. That alone is a very very edge case, and considering the whole bundling/folding process in place in most cases, this is even more rare.

@caridy I couldn't disagree more. I think the way UI development is headed, only way we can reasonably develop/release apps is to hot reload.

In other words, we should not rely on the live bindings mechanism to trigger function calls in consumer module, not without some sort of observable pattern in place to detect those changes.

@caridy I think Chrome already has a way to very nicely observe changes- ever heard of Object.observe ?

m-a-r-c-e-l-i-n-o commented 7 years ago

@dchambers Two years later... What you're describing sounds kind of close to one of my old experiments. Apparently, I wanted to mock dependencies and functions during unit tests, so I thought it'd be a good idea to statically analyze my modules and replace variables with object references that could be manipulated at runtime. As far as I remember, the only limitation is that you can't reassign the variables that you used for the import (i.e. if you import "Foo", you should use Foo throughout the module), but that problem is not THAT hard to solve, either. So @guybedford intentions are theoretically possible this way -- as ugly and potentially convoluted as this approach might be.

dchambers commented 7 years ago

@m-a-r-c-e-l-i-n-o, I've actually never been a fan of unit tests that use mocking frameworks because they almost always end up relying on the private implementation details rather than testing behaviour. Now that I've changed jobs and no longer have lots of legacy tests to maintain, I no longer have these kinds of problems to deal with ... yay! :smile: :tada: