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

Implement hot reloading #46

Closed
gaearon opened this issue Dec 16, 2014 · 46 comments
Closed

Implement hot reloading #46

gaearon opened this issue Dec 16, 2014 · 46 comments

Comments

@gaearon
Copy link

gaearon commented Dec 16, 2014

I implemented react-hot-loader for React (with core being extracted into react-hot-api) and I think it should be possible to implement the same for Cycle, especially considering its this-lessness.

Would you be interested in helping out with this?

@staltz
Copy link
Member

staltz commented Dec 16, 2014

Yes, that would be extremely beautiful, and I thought about it today when having lunch. It should also be very possible in Cycle since everything is an event, so observing DataFlowNodes don't need to care if the observed DataFlowNode is being changed, as long as it issues events.

However, I would leave this issue for a later stage, I want to first work on some issues that may bring major breaking changes to Cycle.

@gaearon
Copy link
Author

gaearon commented Dec 16, 2014

Great, thanks. Let me know when you feel like ready to try!

@ArnoBuschmann
Copy link

Being about to give Cycle a try. While currently using @gaearon s hot loader with react I wonder, if there is a suggestion or an attempt meanhwile on how to get hot loading accomplished in Cycle?

@staltz
Copy link
Member

staltz commented Apr 30, 2015

Good question I haven't given thought to this in a while. Can @gaearon share some basic insights how hot loading works in general? Where does it hook in, and how does it swap old and new code?

@gaearon
Copy link
Author

gaearon commented Apr 30, 2015

I suggest you use Webpack for hot reloading because it's the only bundle system that implements per-module reloading at the moment.

Here are some docs that helped me get started:

http://webpack.github.io/docs/hot-module-replacement.html
https://github.com/webpack/docs/wiki/hot-module-replacement-with-webpack

They are somewhat confusing so if this doesn't help, I'm happy to take a stab at a proof of concept.

The important part here is to first get hot reloading working in a sample project that directly uses Webpack's HMR API, and then figure out how to generate those HMR API calls based on what you know about the modules (e.g. inject HMR API calls into modules that export Streams, or something like that).

@gaearon
Copy link
Author

gaearon commented Apr 30, 2015

Here's a React project boilerpate with hot reloading enabled and set up (ports, server, etc). You can remove react-hot from loaders in webpack.config.js, run it, and then try to use Cycle instead, with explicit module.hot API calls for the time being.

@staltz
Copy link
Member

staltz commented Apr 30, 2015

Thanks a lot for this input!

But, just to make clear, this type of technique is something that works as a separate Webpack plugin, right? It doesn't require changes to Cycle's core? Or, said in a different way, do you think it would be easier to implement hot reloading with React if you could just change some code in React?

@gaearon
Copy link
Author

gaearon commented Apr 30, 2015

But, just to make clear, this type of technique is something that works as a separate Webpack plugin, right? It doesn't require changes to Cycle's core?

Hard to say before trying.
It might need something like uninject or reinject :-).

do you think it would be easier to implement hot reloading with React if you could just change some code in React?

I'm not sure, but I don't think so. Most of the complexity in RHL was driven by the desire to support complex scenarios (e.g. hot-patch component method that was already passed to setInterval), not by React's limitations.

I think Cycle might have it much easier thanks to the lack of this.

@cultofmetatron
Copy link

using webpack's hot reloading. works great so far.

@Widdershin
Copy link
Member

For anyone who wants an out of the box solution.

https://github.com/Widdershin/cycle-hot-reloading-example

The only issue still to be solved really is to restart the application with the same state when the code is changed. I think this can be achieved by implementing an API for drivers to record events from sources, and then allow loading it back in when the app is restarted, potentially by using a VirtualScheduler. Funnily enough, this appears to be the same problem that needs to be solved in order to allow cyclejs/cycle-time-travel#17 to progress.

I'll take a crack at it soon.

@Widdershin
Copy link
Member

@gaearon I'll be having a look at this soon, if you already have any thoughts on how this might be be achieved would love to hear them.

@staltz
Copy link
Member

staltz commented Dec 2, 2015

Niiice! I gotta take a look at this

@Widdershin
Copy link
Member

And now we have a proof of concept for proper hot reloading:

https://gfycat.com/ZealousGreedyKronosaurus

It does require adding a few methods to each driver, namely domDriver.replayHistory() and sources.history() (and probably an argument to makeDOMDriver to enable history recording). I've currently hacked in a very rudimentary version in Widdershin/cycle-dom#history-api along with cycle-restart, but it needs quite a bit of work to be really usable.

I can foresee a few problems with how isolateSource works, but I feel like it's all doable.

@Widdershin
Copy link
Member

@Frikki
Copy link
Member

Frikki commented Dec 9, 2015

@Widdershin That’s sick!!! In a good way.

@erykpiast
Copy link
Contributor

Isn't it totally different approach than in Flux or Redux? It's not only applying new version of application on current state, it's rebuilding state with new code from the same sources like before. I can see huge potential in this! 👍

@Widdershin
Copy link
Member

That's correct @erykpiast. Since your main function should really be a pure function based on the Observable sources, that was the most sensible approach I could come up with. It means you can fix bugs that happened in the past, which is quite cool.

@erykpiast
Copy link
Contributor

Yes, it is! I'm soo excited.

2015-12-09 9:59 GMT+01:00 Nick Johnstone [email protected]:

That's correct @erykpiast https://github.com/erykpiast. Since your main
function should really be a pure function based on the Observable sources,
that was the most sensible approach I could come up with. It means you can
fix bugs that happened in the past, which is quite cool.


Reply to this email directly or view it on GitHub
#46 (comment).

@staltz
Copy link
Member

staltz commented Dec 9, 2015

@Widdershin Nick, this is the moment the world needs you. Go for it.

@Widdershin
Copy link
Member

😄 Thanks for the encouragment @staltz, but I have to sleep first 😉

@staltz
Copy link
Member

staltz commented Dec 9, 2015

Oh yes, New Zealand, I forgot...

@fobos
Copy link

fobos commented Dec 9, 2015

Coool! Glad to see it 👍

@Widdershin
Copy link
Member

Okay, so today I got support for hot reloading in applications that use isolate working. That was the biggest forseeable hurdle in getting hot reloading support.

Still to do:

  • Only record Sink Log in development
  • Write tests for cycle-dom Sink Log API
  • Finalize what the Sink Log API actually looks like
  • Implement Sink Log API in other drivers (definitely cycle-http to start with)
  • Update cycle-time-travel to use Sink Log API directly

I'm keeping track of these issues over on cycle-restart:
https://github.com/Widdershin/cycle-restart/issues

Once all that is done we can consider getting hot reloading merged.

@staltz
Copy link
Member

staltz commented Dec 29, 2015

Great roadmap @Widdershin. When you say "history" I suppose you're not referring to https://github.com/cyclejs/cycle-history right? There might be another name we can give to this.

isolate working. That was the biggest forseeable hurdle in getting hot reloading support.

As a side note, notice how much hurdle a little bit of mutation and impurity can bring. isolate() with an implicit/automatic scope is pretty much the only impure part in Cycle.js, and it's so small. Mutation everywhere is a nightmare. FP: good. 😄

@Widdershin
Copy link
Member

Yeah, I say history because it's a record of all the events coming out of your sources.

We could call it "Hysteresis":

Hysteresis is the time-based dependence of a system's output on present and past inputs. The dependence arises because the history affects the value of an internal state. To predict its future outputs, either its internal state or its history must be known.

Any other ideas are welcome.

@staltz
Copy link
Member

staltz commented Dec 29, 2015

I have a taste for odd names[1], but I try to avoid them because they usually hint very weird and alien ideas when it practice they are pretty normal. I'd call this one the "Sink Log".

a record of all the events coming out of your sources.

I suppose you meant "coming out of your sinks"?

[1] Cycle.js naming has been through some weird shit: BackwardFunction, DataFlowSink, DOMUser, insulate, confine, restrict, Interpreter, etc.

@Widdershin
Copy link
Member

https://github.com/Widdershin/cycle-restart just had it's first release!

After a random comment from @staltz about how I would support simple drivers, I changed up my approach a bit.

I have created a function called restartable. You pass a driver into it, and it gives you back a driver with the following API attached:

driver.onPreReplay()
driver.onPostReplay()
driver.replayLog(log)
sources.log(): log

Where log is an array of occurrences to be replayed, in the format {identifier, event, time}.

That driver can then be restarted, and we can have true hot reloading!

Currently restartable is only tested against @cycle/dom and @cycle/http, and it does require passing in a configuration flag for @cycle/dom. This is what a basic example app looks like:

import {run} from '@cycle/core';
import {makeDOMDriver} from '@cycle/dom';
import {makeHTTPDriver} from '@cycle/http';
import isolate from '@cycle/isolate';

import {restart, restartable} from 'cycle-restart';

import app from './src/app';

const drivers = {
  DOM: restartable(makeDOMDriver('.app'), {pauseSinksWhileReplaying: false}),
  HTTP: restartable(makeHTTPDriver())
};

const {sinks, sources} = run(app, drivers);

if (module.hot) {
  module.hot.accept('./src/app', () => {
    app = require('./src/app').default;

    restart(app, drivers, {sinks, sources}, isolate);
  });
}

If you want to try it out, clone down this github search example and play with the code: https://github.com/Widdershin/cycle-hot-reloading-example/tree/preserve-state

Note that we need to tell @cycle/dom not to pause its sinks while replay is occurring. This (currently) the only information we need to implement replay for a driver.

So! Now we have true hot reloading! Where to from here?

Well, I would really like to make it easy to enable hot reloading in development. To that end, it seems ideal to extend the Cycle.run interface to allow something like: Cycle.run(main, drivers, {development: true}).

Cycle.run would then call restartable on each of the drivers. If a driver needed to give configuration to restartable, it could be exposed as a property on the driver. This seems a small price to pay for reloadability.

Could something like that fit in to the adapters scheme that is currently being worked on?

@staltz
Copy link
Member

staltz commented Jan 15, 2016

I have to emphasize, this is really good work, and highly appreciated @Widdershin.

As soon as I get to experiment with it for a few apps I have and also the community doing the same, we can move it to @cycle/restart or @cycle/recycle and start making it official.

Well, I would really like to make it easy to enable hot reloading in development. To that end, it seems ideal to extend the Cycle.run interface to allow something like: Cycle.run(main, drivers, {development: true}).

I agree, and that's doable. However, does it assume the developer uses Webpack or browserify? Even though these two are our main targets, we want to let it open if people want to use rollup or direct script in the browser.

Could something like that fit in to the adapters scheme that is currently being worked on?

Tangentially, but not exactly. It could fit in a good timing. Since we are reworking the architecture for run(), we can probably put auto-restarting inside base.

@staltz
Copy link
Member

staltz commented Jan 23, 2016

@Widdershin could you give me a hand on getting an example working? Check hot-reloading branch in cycle-examples: https://github.com/cyclejs/cycle-examples/tree/hot-reloading/autocomplete-search

I did some changes, following your https://github.com/Widdershin/cycle-hot-reloading-example, but there are some weird obstacles. Like if (module && module.hot) doesn't pass because module.hot is undefined.

To get cycle-restart in a good shape, we would need some easy setup instructions, and I'm trying to figure that out.

@Widdershin
Copy link
Member

Sure thing, I'll try have a look in the next day or so 😄

@staltz
Copy link
Member

staltz commented Feb 25, 2016

@Widdershin how "ready" do you consider cycle-restart? Do you have additional TODOs in your mind?

@Widdershin
Copy link
Member

I would say 90% ready. I've opened a milestone to track my last few TODOs https://github.com/Widdershin/cycle-restart/milestones/v1.

One of the major pain points at the moment is having to provide options in some cases. It's likely that I'll add additional options in future, and it seems painful to ask users to update. Alternatively, configuration could be part of the driver, but that seems similarly troublesome.

I am considering checking the driver.name and providing default config based on that if none is provided. What do you think of that @staltz? too dirty?

@staltz
Copy link
Member

staltz commented Feb 25, 2016

Sounds a bit dirty, yeah.

One of the major pain points at the moment is having to provide options in some cases.

Can you give an example?

@Widdershin
Copy link
Member

const drivers = {
  DOM: restartable(makeDOMDriver('.app'), {pauseSinksWhileReplaying: false}),
  HTTP: restartable(makeHTTPDriver())
};

Specifically pauseSinksWhileReplaying: false for the DOM driver. Easy to accidentally omit (which will break everything).

@staltz
Copy link
Member

staltz commented Feb 25, 2016

Hmm, yeah I see. driver.name should be flexible. I mean I call it DOM usually but I want to allow people to call it graphics while being the same DOM Driver.

IMO the options are not so bad. Hot reloading is something for development-only anyway, and if it's a matter of one small option, it's ok, as long as it's clearly communicated in README (either of cycle-restart or cycle-dom or both).

@staltz
Copy link
Member

staltz commented Feb 25, 2016

That doesn't stop us from thinking how to solve this, though.
I'd suggest we consider it an improvement, not a show-stopper.

So if I rephrase: do have anything TODO until cycle-restart is 1.0 or "ready for primetime" (whatever how they call it)?

@Widdershin
Copy link
Member

The README is the biggest thing that needs to be done.

I am also aware of one bug where cold drivers are indiscriminately converted into hot drivers, which causes some issues. That doesn't seem to impact any of the common drivers though so it's not as big of a priority.

@staltz
Copy link
Member

staltz commented Feb 26, 2016

I removed "help wanted" just so people don't think this issue still needs an assigned leader. Because we already got one. :)

staltz pushed a commit that referenced this issue Jul 18, 2016
@staltz staltz added the size L label Jul 25, 2016
@staltz
Copy link
Member

staltz commented Aug 30, 2016

This seems to be so userland, so old, so vague, that I think we can safely close it.

Nothing stops us from using this space for discussions though.

@staltz staltz closed this as completed Aug 30, 2016
@Widdershin
Copy link
Member

Agreed, I will open new issues if there's anything that needs to be changed
in Cycle to accommodate hot reloading

On 31/08/2016 11:09 AM, "André Staltz" [email protected] wrote:

This seems to be so userland, so old, so vague, that I think we can safely
close it.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#46 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAYUHci4fNdqmolK2xxONGCZ7YehIGstks5qlJbfgaJpZM4DI_Hj
.

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

No branches or pull requests

10 participants