Skip to content

Make app return kill func #873

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

Closed
wants to merge 1 commit into from
Closed

Make app return kill func #873

wants to merge 1 commit into from

Conversation

zaceno
Copy link
Contributor

@zaceno zaceno commented Aug 15, 2019

Main idea: make it possible for apps to die like things usually do: the corpse (the DOM nodes) are still there, but do nothing. Subscriptions are shut off as well.

usage:

const kill = app({...})
setTimeout(kill, 5000) //the app just stops doing anything after five seconds

There was a thought to return an object, where kill would be the only prop, as a way of future-proofing (in case we want to add other things to app's return in the future, without breaking existing code).

I think it's a good idea but could use some discussion, so for this initial submission I decided to just return the kill function, sidestepping the naming discussion and keeping the bundle-size addition minimal.

@codecov-io
Copy link

codecov-io commented Aug 15, 2019

Codecov Report

Merging #873 into master will decrease coverage by 0.2%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #873      +/-   ##
==========================================
- Coverage   19.45%   19.25%   -0.21%     
==========================================
  Files           1        1              
  Lines         185      187       +2     
==========================================
  Hits           36       36              
- Misses        149      151       +2
Impacted Files Coverage Δ
src/index.js 19.25% <0%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0764036...84638ea. Read the comment docs.

@zaceno
Copy link
Contributor Author

zaceno commented Aug 15, 2019

Added node=null to make sure we don't keep the detatched DOM in memory longer than needed. But we return it from the kill function so post-processing can be done if needed.

@zaceno
Copy link
Contributor Author

zaceno commented Aug 15, 2019

Actually, let’s forget about those last two commits. Sorry! I’ll revert them first thing tomorrow. Short story: memory is only kept for as long as reference to kill function is kept. So no need to null the node.

@jorgebucaran
Copy link
Owner

jorgebucaran commented May 15, 2020

@zaceno Could you elaborate on how and why this works?

Also, can we always cut subscriptions off like this or would we benefit by having something like:

const Goodbye = (state) => ({ ...state, isRunning: false })

const kill = app({
  node,
  init,
  done: Goodbye,
  subscriptions: (state) => [
    state.isRunning && [
      ping,
      {
        host,
        gateway,
        timeout,
        onRequestResponse,
      },
    ],
  ],
})

Finally, what does dispatch = Function do?

@zaceno
Copy link
Contributor Author

zaceno commented May 15, 2020

@jorgebucaran

@zaceno Could you elaborate on how and why this works?

Sure!

Ok, so what the point of this is, is to basically make an app stop doing anything and to make it stop being interactive. We do want to leave the nodes as they are, because we might want to use them for something (like perhaps resurrecting the app).

If we want to remove the nodes that's easy enough in userland. The stuff this PR does is not doable in userland.

First, we want to make sure that we can't interact with the elements in the app. Rather than rerendering or removing all the event handlers we simply set dispatch=Function, effectively making dispatch a no-op. Click buttons all you want and nothing will happen.

Furthermore this makes sure that effects and subscriptions that get the idea to dispatch something after the app has been killed, won't cause runtime errors.

But subscriptions also typically hold on to their event-listeners. If you are starting and stopping apps a lot like in a customElement scenario, that's a memory leak. So we also want to make sure to unsubscribe everuthing (even though nothing can be dispatched).

That's what patchSubs(subs, EMPTY_ARR) does.

As for the done prop, I don't know. I don't really see why that would be needed. Maybe you can explain?

@zaceno
Copy link
Contributor Author

zaceno commented May 15, 2020

Oh wait I see. You want to do something when the app stops? In principle, shouldn't that be an effect (=output) rather than a subscription (=input) ? At least if you're doing it from inside the app. If you're doing it from outside, you could do it with vanilla js

@jorgebucaran
Copy link
Owner

jorgebucaran commented May 16, 2020

How important is it we dispatch=Function and could we get away without it?

Here are some ideas. No changes to core, but we need an id so that my end and kill sub/fx pair can communicate:

import { app } from "hyperapp"
import { end, kill } from "./fx"
import { initialState } from "./state"

const Done = (state) => ({ ...state, isRunning: false })

app({
  init: (defaultProps = {}) => ({ ...defaultProps, ...initialState }),
  subscriptions: (state) => [
    [end, { id: 666, Done }],
    state.isRunning &&
      [
        /*...*/
      ],
  ],
})

// Moments later...
kill(666)

Or with minimal changes to core, app could return this "app id" that we also want to pass to init via props:

import { app } from "hyperapp"
import { end, kill } from "./fx"
import { initialState } from "./state"

const Done = (state) => ({ ...state, isRunning: false })

const id = app({
  init: (defaultProps = {}, id) => ({ ...defaultProps, ...initialState, id }),
  subscriptions: (state) => [
    [end, { id, Done }],
    state.isRunning &&
      [
        /*...*/
      ],
  ],
})

// Moments later...

kill(id)

Or simpler, but riskier, just return dispatch from the app:

import { app } from "hyperapp"
import { initialState } from "./state"

const Done = (state) => ({ ...state, isRunning: false })

const dispatch = app({
  init: (defaultProps = {}) => ({ ...defaultProps, ...initialState }),
  subscriptions: (state) => [
    [end, { Done }],
    state.isRunning &&
      [
        /*...*/
      ],
  ],
})

// Moments later...

dispatch(Done)

@zaceno
Copy link
Contributor Author

zaceno commented May 16, 2020

How important is it we dispatch=Function and could we get away without it?

dispatch=Function Is the “cause of death” that is how we effectively kill the app. Even any outstanding fetch-calls that come back with data will not cause any “death spasms” with that.

Your approach doing it all in userland all requires the app to work through one last render which doesn’t work in the scenario of customElements (which is what I was working on when I came up with this). There we can’t let the app render one final time because the node has been removed from the dom and weirdness ensues. That’s why dispatch=Function. To make absolutely sure nothing tries to modify the DOM once the kill function is called.

Also, since in your solution, you do not disable dispatch, any effects waiting for something before they dispatch an action will cause “haunting” (the app will do something even after its supposed to be dead)

@zaceno
Copy link
Contributor Author

zaceno commented Nov 27, 2020

Can I bump this for consideration, since there aren't any plans (afaik?) to return anything from app()?

Being able to kill an app cleanly and immediately is useful when using a multi-app architecture, and when implementing custom elements.

Yes it is possible to use middleware + HOA emulate this but it requires an extra render (since we can't get att the patchSubs function from the ouside). That makes any DIY solutions less elegant and potentially bug-inducing. Also it breaks the custom element use case since the node will be gone when we want to kill the app.

Those things might be considered edge cases but whatever: I feel that Hyperapp is incomplete if it can't clean up after itself without relying on the DOM, basically.

@jorgebucaran
Copy link
Owner

@zaceno Do you have a link to a code example that would rely on a kill function, it could be that custom elements pen you created at some point. I know I've seen it, but I can't find it anywhere now.

@zaceno
Copy link
Contributor Author

zaceno commented Nov 28, 2020

@jorgebucaran sure! That custom element pen isn't working anyway anymore, but I stripped down some code I was working on at the moment, to some examples that demonstrate the problems with stopping apps currently. Right now I'm working on an update to my sevenguis examples, and I wanted to make it like an app gallery (also as a proof of concept of how to do that in general).

First have a look at this example. It is a simple gallery which lets you run a counter or a clock. The clock has a subscription to every, and on each tick it console logs 'TICK'. Notice that each time you switch over to the clock, it will start logging a new series of 'TICKS' – more and more, never stopping.

Clearly we need the ability to stop an app when we switch a way from it. So in this example I use a custom app function to run the apps that returns a stop function (best I can do from the outside). This does indeed make the "TICK"ing stop.

But now I am getting errors each time I switch (null is not an object (evaluating 'state.count')) – because it is trying to render one extra time with the "stopped" state – which does not match what the view expects.

I attempt to fix that in this third example by making my custom app function handle the stopped state, and render a single, empty div instead of the declared view. Somehow that causes inscrutable runtime errors in hyperapp.

I can only conclude that properly stopping an app "from the outside" is not feasible, we need hyperapp to support a way to do that without causing an extra render.

@jorgebucaran
Copy link
Owner

jorgebucaran commented Jan 25, 2021

A variation of this proposal would be to modify dispatch so that dispatch() works like your kill function.

+    : action == null 
+      ? patchSubs(subs, EMPTY_ARR, (dispatch = id))
       : setState(action)
)

I've always felt like we should probably return dispatch or at least something more useful than undefined. This was always on my radar. We could achieve that and still have a way to terminate apps at once.

With dispatch in hand, you have absolute flexibility over the termination process, which may be useful. Heck, there may be a situation where you want subscriptions to run one more time before terminating the app. You may want to do one more thing, before passing on.

Userland dispatch would be a dev-feature we could also use for tooling, automation testing, integrating tests, etc.

@zaceno
Copy link
Contributor Author

zaceno commented Jan 25, 2021

CLosing in favor of #1018

@zaceno zaceno closed this Jan 25, 2021
jorgebucaran pushed a commit that referenced this pull request Jan 25, 2021
1. Return dispatch so it could be used for tooling,
   automation, tests, and so on.
2. Make dispatch with no arguments stop the app (#873).

Stopping an app means that future calls to dispatch do
nothing and all active subscriptions are unsubscribed.
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

Successfully merging this pull request may close these issues.

4 participants