State Hydration Proposals/Discussion

[peteruithoven]

As I can't comment on he readme I'm using this issue.

Great write up of Javascript Plugin Reload Hook solutions.

I wrote the original __reload hook (alexisvincent/systemjs-hot-reloader#23) but I agree that this complicates things, as it runs after the regular initiation is run, you'll need hooks to adapt what's already initiated (like Redux's replaceReducer).

You might find the following discussion interesting: alexisvincent/systemjs-hot-reloader#34

For example Proposal #3 would give you access to the old module, but a good point of mikz is that this requires you to make your state public (export it).

I like my last idea: alexisvincent/systemjs-hot-reloader#34 (comment), which I turned into a simple utility: simple systemjs-hot-reloader-store utility. This enables you to use the old state during the regular initiation of the module and doesn't require exporting the state. It also doesn't require any special hooks.
It's big downside however is that you manually need to pick a unique name.

Maybe there is a way to enable every module to import a @hotState, which is a
unique object per module. This can then used to store state in and on reload, when it's there, it can be used for rehydration. This fixes the unique name issue and the module author can decide if he want's to store anything and what is stored. (I stole the name from: alexisvincent/systemjs-hot-reloader#34 (comment))

[alexisvincent]

@peteruithoven Thanks for taking the time to comment! Also, thanks for the link, I had no idea people had been discussing this.
After reading through everything, I really your @hotstate idea!
In the simplest case I think each module should have access to a persistent private object, people can use, or not. Something like:

import module from '@module'

//this idea could be extended in a few ways by

//exporting the unload function here instead of globally
module.unload = function() {
    // do something. Notice we no longer need access to the oldModule as entry... Since it is in scope.
}

//we could pass in other information (not claiming this is a good idea, but) similar to node
//https://nodejs.org/api/modules.html#modules_module_filename
module.filename == 'someFile'
module.children // dependents

But even if module (or some other name) is just an empty object, this would solve the use case beautifully, and I don't think this would be hard to implement via a custom loader either. Depending on whether or not the loader knows which file the import originated from. I suspect not. But either way. This would be an incredibly valuable extension.

Finally, in terms of GC, i think the things being persisted to this store (@module) would be precisely the things the developer means to persist (and would do via another method if not here).

[peteruithoven]

Notice we no longer need access to the oldModule as entry... Since it is in scope.

As in, the previous module would be this?

How would someone access his old state from module?

I would personally keep the content of that persistent private object up to the developer. I would certainly not automatically store a reference to the whole module, since that might encourage exporting the state, just for hot reloading, which I feel is bad practice.

Being able to retrieve extra information about the module sounds interesting, but I wouldn't mix that with persistent storage. Maybe two possible imports could be provided; @module and @hotState.

Not that I'm against but, why would you put the reload hook in module?

[alexisvincent]

I would personally keep the content of that persistent private object up to the developer. I would certainly not automatically store a reference to the whole module, since that might encourage exporting the state, just for hot reloading, which I feel is bad practice.

I'm not suggesting we store a reference to the whole module. Only that it is a private store available for write. Perhaps module would be a bad name, since it leads to this confusion.

Being able to retrieve extra information about the module sounds interesting, but I wouldn't mix that with persistent storage. Maybe two possible imports could be provided; @module and @hotState.

This could work

As in, the previous module would be this?
How would someone access his old state from module?
Not that I'm against but, why would you put the reload hook in module?

The reason the unload hook might work better in the '@hot' module would be that then its not being exported to the whole world. With regards to getting previous state, this would amount to accessing data stored in @hot. So:

import hot from '@hot'

const incrementNumber = () => {
    hot.numberToPersist++;
}

//here we store some data we want persisted
if (hot.reloading) {
    console.log("reloading with number ", hot.numberToPersist)
} else {
    //here we store some data we want persisted
    hot.reloading = true
    hot.numberToPersist = 42

    hot.unload = function() {
        incrementNumber()
        // As far as I can see we don't need access to the 'old module'. 
        // Since we have the hot store. Which can be used to do the same thing really.
        // And like this we don't export unload to the whole world. 
        // Just a thought...
    }

    console.log("first load with number ", hot.numberToPersist)
}

console.log("initializing with number ", hot.numberToPersist)

Sorry, I know this example of usage is messy. But I wanted to show usage from a number of cases.

[peteruithoven]

Like!
I like simply @hot, I get your hot.unload preference.

[alexisvincent]

Awesome! Maybe we can get some comment from our sensei @guybedford

[alexisvincent]

ping @guybedford

[guybedford]

Ok, I appreciate the argument that the __hotReload hook runs after execution, and so results in a double init. We should get access to the old module from the first step of execution.

Then the only way to get this would be if the old module itself was available through an import, or an import API. We can't have the old module available as an import though, because it is undefined when starting up, so the remaining option is an API to get the previous instance.

The problem with an implicit @hot import is that it would effectively normalize into a unique name like normalized-module-name@@hot or something like that. We're effectively doubling the size of the module registry, which is probably not a good idea for performance. Some global @hot import that just works any other way would be a hack of the module system.

I guess I do think an API would be the best alternative. The issue with a state store, is that then you do just have a generic state store - the problem space completely changes from storing the previous instance state, to all the Redux-ish problems of ensuring a good state store API. It becomes an easy slope where the API suddenly becomes a source of truth of application state when all it was supposed to be was a hot reloading interface.

So having the ability to get the previous module instance with publicly exported state, or at least a public "getState" function (which is probably the best practice anyway), would be the better way to go about this. Also by having a readable state only, it avoids further abuse.

It would be nice if this could just work via:

var previousInstance = System.get(__moduleName);

let previousState = previousInstance.getState();
previousInstance.unload();

But we do need to delete the previous instance from the registry before kicking off the loading process unfortunately.

So perhaps just something like:

import { getPreviousInstance } from '@hot';

let previousInstance = getPreviousInstance(__moduleName);
let previousState = previousInstance.getState();
previousInstance.unload();

It would be possible for getPreviousInstance to work without the __moduleName argument being necessary by relying on synchronous execution being triggered by the hot reloader itself, something like:

hot-reloader-internals.js

var instance;
function getPreviousInstance () {
  if (!instance && instance !== false)
    throw new ReferenceError('getPreviousInstance can only be called during immediate module execution');
  return instance;
};

function hotReload(moduleName) {
  var m = System.get(moduleName);
  System.delete(moduleName);
  System.load(moduleName).then(function() {
    // this triggers execution - load is non-executing
    instance = m || false;
    System.get(moduleName);
    instance = undefined;
  });
}

I can't drive any of this work though, so take what you want from all that - no permission is needed :)

[alexisvincent]

@guybedford Thanks for taking taking the time to answer. I see your point about doubling the size of the module registry, and I agree, the import semantics we were speaking about do feel hacky.

I think the best option here would be to expose a getPrevious function from '@hot' which accepts a module name. While this isn't as clean as just importing the previous module, I think this better respects the module loader. As for the API that would allow calling getPrevious without the module name, I'm not keen on taking over the loading process of SystemJS. Im specifically worried about what it would mean for handling circular dependencies, but maybe this isn't as much of a problem as I think it is.

@peteruithoven what are your thoughts on the following API.

import { getPrevious } from '@hot'

const previous = getPrevious(__moduleName)

// State hydration could be handled like this
const state = previous ? previous.state : {}

// people could define their own unload rules via
if (previous)
  previous.unload()

const unload = () => { console.log("I just unloaded something") }

export { unload, state }

Allowing the user to handle their own unload logic is nice, but I still think we might need to allow them to export a named function (maybe__unload) that we will execute upon deletion. Thoughts?

Even if this isn't the case for JS. I think loaders should be able to describe an unload process as described here https://github.com/alexisvincent/systemjs-hmr#loader-plugin-unload-hook.

With regards to implementing the above, I'm thinking that getPrevious should have access to a registry where we save old module instances on deletion, @peteruithoven similar to your store, but where the 'name' is moduleName.. This means that getPrevious will always return the 'last' instance of a module, regardless of where it is called (e.g. outside of 'immediate module execution'). We can avoid storing old instances for all modules by only saving a copy if the module directly depends on @hot.

This is rather cool as it simplifies how HMR is currently performed and is really simple to implement.

[alexisvincent]

@peteruithoven thoughts?

[peteruithoven]

The problem with an implicit @hot import is that it would effectively normalize into a unique name like normalized-module-name@@hot or something like that. We're effectively doubling the size of the module registry, which is probably not a good idea for performance. Some global @hot import that just works any other way would be a hack of the module system.

Are there no hooks that allow checking something before checking the module registery? Just like there are hooks that allow transpilation or loading files from other places for example.

I guess I do think an API would be the best alternative. The issue with a state store, is that then you do just have a generic state store - the problem space completely changes from storing the previous instance state, to all the Redux-ish problems of ensuring a good state store API. It becomes an easy slope where the API suddenly becomes a source of truth of application state when all it was supposed to be was a hot reloading interface.
So having the ability to get the previous module instance with publicly exported state, or at least a public "getState" function (which is probably the best practice anyway), would be the better way to go about this. Also by having a readable state only, it avoids further abuse.

I don't really see the problem of "a generic state store", and what Redux-ish problems are (I'm a Redux user btw). Especially since it's so similar to how my simple systemjs-hot-reloader-store utility works.
I feel that being able to store a specific peice of data is more flexible and controlled than forcing to go through the previous module instance, forcing the need to add an extra public export to the module.

getPrevious(__moduleName)

Are we sure there is no way to have @hot figure this out automatically?

[alexisvincent]

@peteruithoven I just checked the module loader spec and it turns out that the loader hook accepts as an argument the name of the importer module. Meaning that this whole thing can probably be implemented as a loader plugin. The only complication would be that the loader would need access to a global registry, but if there are no issues with loading order this should be pretty simple.

This allows us to do a number of things.

We can import module directly here, since the '@hot' loader knows which module this is.
Which would shorten my last example to the following

import { module } from '@hot'

const __state = module ? module.__state : {}

if (module)
  previous.__unload()

const __unload = () => { console.log("I just unloaded something") }

export { __unload, __state }

we could also

import { module, state } from '@hot'

if(module) {
    // function exported from module
    module.__unload()

    // function set on state
    state.__unload()

    console.log('reloading')
} else {    
     console.log('initial load')
}

// unload function exported on state
state.__unload = () => {
    console.log("unload function from state")
}

// unload function exported on module
const __unload = () => {
    console.log("unload function from module")
}

export { __unload }

I'm slightly more wary of the second one (even though this was my first choice before), since people might misuse it, as Guy said:

It becomes an easy slope where the API suddenly becomes a source of truth of application state when all it was supposed to be was a hot reloading interface.

On the other hand, exporting state you want to persist isn't "nice". Another issue I can see when relying on the first step would be the fringe case where people export a bunch of functions from a module (including __state/__unload/whatever) and then another module wants to

import * as something from './something.js'

something.forEach(f => f())

While this is a fringe case, it at the very least prevents folks from implementing a pattern like this from that module. I really doubt this is an issue, but just throwing it out there.

@guybedford comment?

[peteruithoven]

The loader plugin, how would it retrieve the module name? If it has the module name, why does it need the global registry?
If it could be a loader plugin then it could be a separate package altogether, making it less of a problem when it becomes the source of truth, since it's less connected to hot reloading.

[alexisvincent]

Sorry, I meant to say the normalize hook.

So the code I'm suggesting would look very roughly as follows

System.set('_hot', loader.newModule({
        repository: {},
        meta: {}
})

const oldNormalize = System.normalize;

/*
 * name: the unnormalized module name
 * parentName: the canonical module name for the requesting module
 * parentAddress: the address of the requesting module
 */
function normalize(name, parentName, parentAddress) {
    System = this;
    // Or something to this effect
    if (name == '@hot')
        return oldNormalize(parentAddress + parentName) + "@hot"
    else
        return oldNormalize(name)
}

function instantiate(load) {
  // an empty return indicates standard ES6 linking and execution
  return;

  moduleRepo = System.get('_hot').repository[load.name]

  // a return value creates a "dynamic" module linking
  return {
    deps: [],
    execute: function() {
      return loader.newModule({
        module: moduleRepo.module,
        state: moduleRepo.state
      });
    }
  };
}

[alexisvincent]

Ok, so I've written and published a complete rewrite of SystemJS HMR (significantly simpler).
@guybedford Is what I'm doing here 'safe'?

systemjs-hmr 'next' supports the following api(s)

import { module } from '@hot'

export const state = module ? module.state : {}

// module == false on first load
if(module)
    module._unload()

export const _unload = () => console.log('unload something')

Note that unload is not an API I expose, but rather a simple export.

You can also

import { _state } from '@hot'

// state == {} on first load
_state.something = 'something'

Where state is a persistent object (you will get the same object back on every reload). However this is just here for experimentation. It is not likely to stay unless someone can convince me to keep it.

To use the new systemjs-hmr, simply jspm install npm:systemjs-hmr and

import 'systemjs-hmr/dist/next'

I have submitted a PR to systemjs-hot-reloader. And once that is approved you will be able to

import 'systemjs-hot-reloader/next/default-listener.js'
or 
import HotReloader from 'systemjs-hot-reloader/next/hot-reloader.js'

[alexisvincent]

README has been updated to explain how this works.

[alexisvincent]

@guybedford, would really appreciate your input here

[guybedford]

@alexisvincent the approach can certainly adapt to new changes in SystemJS, just note that every major may break the API you are relying on. I'm happy to assist with the upgrade paths as we hit them if you have any questions.

[alexisvincent]

Great thanks!