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

Hot-reloading #688

Open
1 of 5 tasks
OhadRau opened this issue Dec 28, 2019 · 2 comments
Open
1 of 5 tasks

Hot-reloading #688

OhadRau opened this issue Dec 28, 2019 · 2 comments
Assignees
Labels
A-infrastructure Area: Project infrastructure, build system, Ci, website etc. A-other Area: Other, does not fit neatly into any of the other buckets enhancement New feature or request help wanted Extra attention is needed

Comments

@OhadRau
Copy link
Collaborator

OhadRau commented Dec 28, 2019

Hot-reloading has been discussed as a possible feature for quite some time and I wanted to go ahead and create a permanent discussion about it here in the issue tracker.

So far, what we've discussed has been very similar to what Reprocessing does, which consists of watching the code for changes & recompiling + relinking it with Dynlink.

However, in order to be a competitive UI framework, we would prefer stateful hot reloading like what Flutter currently has. Because the UI is a pure function of state, hot-reloading is safe to perform in Flutter (and should be in Reason as well!). The goal here is to store the state of the program (which should be persistent) independently from the UI rendering (which should be volatile), so that upon hot-reloading the old state is recovered and the UI continues rendering as if nothing happens.

In order for that to work, we need some kind of semantic data about what the state is, so that the module knows how to relocate it. One solution for this is creating a centralized database (key-value store) of states with a unique tag for each state. When the application is launched, every state can query the database to see if it has an existing entry that it can adopt; otherwise it would use the default initial value. In the case where the state's type or name changes, the tag would no longer match so it would just use the default value. Reading and writing would happen directly within the DB so there would be no need to copy state back and forth.

To implement this system, it seems like we want some way of giving states an identifier. While this is a lot of extra work for the user to juggle these IDs, a fork of the brisk-reconciler PPX could generate these at compile-time and pass them in without the user having to worry about them. Note that this identifier has to be unique between different invocations of the same component, so if I created 2 checkboxes I need to make sure that their state is separate; at the same time, it has to be mapped back to the right checkboxes upon reloading. It is most likely not possible to preserve the state based on the order these components are rendered, so we need to be really smart about how to "hash" the component.

In terms of a roadmap, I think we'd need the following features for this system to work:

  • Get bytecode builds working (Enable bytecode builds #693)
  • Switching the way that apps start up to track whether the app is just reloading or being created for the first time (App.start can probably handle this opaquely)
  • A "hotlink" library of some kind that manages the watch/recompile/relink phase
  • Implementing the tagged state DB so that states persist between reloads
  • Using the %hook PPX to insert state identifiers
@OhadRau OhadRau added enhancement New feature or request help wanted Extra attention is needed A-infrastructure Area: Project infrastructure, build system, Ci, website etc. A-other Area: Other, does not fit neatly into any of the other buckets labels Dec 28, 2019
@zbaylin zbaylin self-assigned this Dec 28, 2019
@OhadRau OhadRau self-assigned this Dec 28, 2019
@bryphe
Copy link
Member

bryphe commented Dec 28, 2019

Thanks for thinking about this, @OhadRau ! We totally need this

Some ideas from Discord:

The API I was thinking for hotlink would be something like:

let ui: ref(state => React.element) = hotlink
(
   ~onReloading=() => print_endline("Detected change..."),
   ~onReloaded=() => print_endline ("Reloaded UI"),
   ~onError=(err) => print_endline ("Error loading dynlink lib: " ++ err),
   "Example_UI", // In native - picks up 'Example_UI.cmxs'. In bytecode - picks up 'Example_UI.cmo'
);
...

Then we can use the let newUi = ui^(state) and do the reconciliation
(We'd want to plumb in the onReloaded event to Revery to trigger a new render)

But the API would be pretty straightforward to implement on top of:

  • file watching (@ulrikstrid's fswatch libs)
  • dynlink API
  • dune watch command

The main constraint being that the loaded cmxs and the runtime must agree on the state type and the React module

In addition, I think one very first step called out by @Et7f3 is to set up byte-code builds. These are slower in runtime performance, but much faster to build - so whatever route we go for hot-reloading, it will improve the dev-iteration time 👍

For a more concrete example - we could slightly tweak our UI in Onivim 2:

let ui = hotlink(
~onReloaded=() => dispatch(Actions.Reloaded), // Just trigger a new render
"Oni_UI"
);

And change our function here:
https://github.com/onivim/oni2/blob/master/src/bin_editor/Oni2_editor.re

from:
let update = Revery.UI.start(w, <Root state=currentState^ />);
to:
let update = Revery.UI.start(w, () => ui^(currentState));

One challenge with reloading the UI 'function' is that we don't have a way to know if hooks changed. Consider the following scenario:

let%hook (state, setState) = Hooks.state(...)
let%hook () = Hooks.effect()

...

let%hook () = Hooks.effect()
let%hook (state, setState) = Hooks.state(...)

We have no way of knowing that the hooks changed order, and thus, the backing objects will be incorrect (and probably crash). So for this, the safe thing to do would be to have a 'fresh' render. Which I think is reasonable to start. We could look at ways to improve this as a next step.

To have a 'minimal viable product' of this feature, I think we could start with:

Part 1: Just restart entire app:

  • Having the ability to switch to bytecode builds
  • Run dune watch, and just reload app every time

Part 2: 'hot-link' parts of the app:

  • Bring in the hotlink API for UI

Part 3: finer-grained state reloading:

  • Handling of hooks changes

Switching the way that apps start up to track whether the app is just reloading or being created for the first time (App.start can probably handle this opaquely)

I'm not sure that this is needed - from the roadmap I see, we could start by just actually reloading the app. Then, when we move to the finer-grained hot reloading, like hotlink, the app will always be started - it's just pieces inside change.

@OhadRau OhadRau removed their assignment Dec 29, 2019
@Et7f3
Copy link
Member

Et7f3 commented Jan 17, 2020

Ok so some update on the work done:

Having the ability to switch to bytecode builds
This is done by @OhadRau
We have esy run that will execute the app in native, we have esy run-bytecode that run in dev and bytecode mode, esy run-native to run in dev and native mode and also esy build-bytecode I thought it was useful to rebuild the change on the side but I had used a different approch (i.e. it is the main function that build and link onIdle)
Run dune watch, and just reload app every time
The onIdle event is triggered easily when no animation occur and I switch from my editor to the program so I have decided to use this.
Also I have extracted the main part UI.start and App.init in a specific file so I can call dune build path/lib_view.cma and dynlink that.
Bring in the hotlink API for UI
in the toplevel component file I call
Revery_Core.Event.dispatch(Revery.UI.hotReload, ()); with force rerender. The api is still raw at the moment but enough to dev with it.

Handling of hooks changes
For this I need some direction/help, I don't know what to choose.

the current state: I have forked brisk-reconciler and added 2 commits. To explain: We have Main.re that contain the Toplevel component. and Component.re that contain Component1 and Component2. and our UI is.

<Toplevel>
    <Component1 />
    <Component2 />
</Toplevel>

We have 3 implementations:

  • Et7f3/brisk-reconciler@0a2e476: This commit is the one we use on master: each component are different so on reload the old Toplevel is unmounted and the new Toplevel is mounted so all effect hook are run and state is discarded. It is good for app like Oni where the state is store outside the Toplevel component.
  • Et7f3/brisk-reconciler@15242c3: The rule is easy: If two component have the same name their are equal + the old behaviour. I hash the debugName added by the ppx to ensure no clash happen. User shall use the brisk-reconciler.ppx so no problem should occur.
  • Et7f3/brisk-reconciler@872577b: In the old commit if a component change his hook list we get a runtime error and the app exit. Now if we edit Component.re Component1 and Component2 are considered to be different of the old component (this is because they are in the same file). In fact I add the time of the build to the debugName. So each time we modify a file we umount all the component of that file. And mount the new ones.

Another idea is on hot-reload is detected: compare the hook element is they need the same hook list. ATM we can check at runtime state -> ref != ref -> state but we can't detect change from state(int) -> state(float) to state(float) -> state(int) because the brisk-reconciler use a diff list so it has a variant for all the hook available. OCaml discard type so int and float are lost. We can use Obj module to guess the type but we can't differ between abstract type and even if we can detect this type change we can detect when 2 hook of same type are swapped.

let%hook var1 = Hooks.state(0);
let%hook var2 = Hooks.state(0);

in

let%hook var2 = Hooks.state(0);
let%hook var1 = Hooks.state(0);

(this case is valid at runtime but can be seen has bug) another case is to detect some HOT_RELOAD env variable and do transformation based on this like add a identifier. I don't know if it will be feasible to have different type based on the env. We can always do that but I don’t want that the hot relod impact optimized run.

Also to test on a real project and finish the 7guis challenge: I have tried to set up hot-reload for a external project and it failed ☹️ you can see https://github.com/Rolltrax/Revery7GUIs/tree/Et7f3-setup-hot-reload where I try to enable the hot-reload. The current error is

Fatal error: exception Dynlink.Error (Dynlink.Private_library_cannot_implement_interface "Lib_view")

I don't understand it because it work in the example app here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-infrastructure Area: Project infrastructure, build system, Ci, website etc. A-other Area: Other, does not fit neatly into any of the other buckets enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants