jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.06k stars 781 forks source link

Merge hooks and subscriptions into "events". #174

Closed jorgebucaran closed 7 years ago

jorgebucaran commented 7 years ago

I'd like to simplify the API a bit more without losing functionality / power or compromising speed. I think this is possible by removing subscriptions and hooks and introducing a new concept: events.

app({
  events: {
/* replaces subscriptions*/
    onLoad: [ (state, actions), ... ], 
/* below are the hooks you already know */
    onAction: [ (state, action, data), ... ], 
    onUpdate: [ (oldState, newState, data), ... ],
    onRender: [ (state, actions, view), ... ],
    onError:  [ (state, error), ... ]
  }
})

Example 1

Before

app({
  subscriptions: [
    (model, actions) => actions.doSomething()
  ]
})

After

app({
  events: {
    onLoad: (state, actions) => actions.doSomething()
  }
})

Example 2

Before

app({
  hooks: {
    onAction: (action, data) => console.log(action, data)
  }
})

After

app({
  events: {
    onAction: [
      (state, action, data) => console.log(action, data)
    ]
  }
})
zaceno commented 7 years ago

How does the plugin concept play into this?

zaceno commented 7 years ago

And also the talk of the onRoute hook? (Maybe a dumb question - haven't been following that very closely)

jorgebucaran commented 7 years ago

@zaceno Plugins would continue to work as usual, they should be able to add new events, just like they can attach multiple hooks. This is just another name overhaul, trying to make the API smaller and neater.

If this goes through we'd only have:

FlorianWendelborn commented 7 years ago

@jbucaran In theory events could be predefined actions (actions.on.load). Only issue would be that actions would need to be able to handle arrays then.

Works well with the signature normalization I suggested:

onLoad(model, error, actions)
onRender(model, view, actions)
onAction(model, {name, data?}, actions)
onUpdate(model, {new, old, data}, actions)
onError(model, error, actions)

Would also implicitly enable plugins to expose new hooks, like actions.on.route or actions.router.onRoute.

arturi commented 7 years ago

I’m all in favor of simplicity :) What’s your take on the recent release of Choo, which basically exposes event-emitter and lets you define your own actions and listen to built-in ones like DOMContentLoaded wherever you want in your app? That’s roughly what we’ve been doing in Uppy, and what Substack advocates for.

nichoth commented 7 years ago

@arturi how has the event emitter pattern been working in your app? I haven't used that architecture, but I think you would need a good helper/pattern to keep event namespacing comprehensible in a large application.

arturi commented 7 years ago

So far all right :) Haven’t had problems, but I realize we might in the future. Using namespace-emitter.

jorgebucaran commented 7 years ago

In theory events could be predefined actions (actions.on.load).

That's too wild. In my head actions are functions you call and events functions that we/hyperapp call/s. I see what you mean, but let's keep it simpler.

Would also implicitly enable plugins to expose new hooks, like actions.on.route or actions.router.onRoute

This would be possible anyway with a small change when hooks are initialized and yes, we'll have onRoute :)

Basically:

if (obj = plugin.hooks) {
  Object.keys(obj).forEach(function (i) {
    (hooks[i] ? hooks[i] : hooks = []).push(obj[i])
  })
}
jorgebucaran commented 7 years ago

What’s your take on the recent release of Choo

Quite intriguing, but everything changed!

To be honest, not really excited about mutable state and whatnot.

FlorianWendelborn commented 7 years ago

@jbucaran Can we take this as an opportunity to also allow hooks to be arrays? (like subscriptions were and like hooks are now when using plugins)

Might already be possible, haven't tried.

jorgebucaran commented 7 years ago

@dodekeract I tried to make them all arrays (so arrays of arrays), but couldn't find an easy way to squeeze it in. I'd be open to review a PR, but the idea was to simplify, not complicate more.

zaceno commented 7 years ago

👍 for the idea of mergeing subscriptions and hooks into "events.

I basically like @dodekeract's signature normalization, except I think perhaps:

a) If we give all events the model (the current model) as first argument, then onUpdate doesn't need new in the second argument

b) Wouldn't it make sense to provide the "error" function to all the events as well (except for onError of course 😉 ) ?

FlorianWendelborn commented 7 years ago

@zaceno I think we should even provide it to error, just for the sake of consistency. If a user exploits this to create an infinite loop we can't prevent it anyway. Users can always write while (true) {}.

zaceno commented 7 years ago

@dodekeract. Sure I agree we don't need to protect users from themselves ;) But I wonder if there's any way that error could be actually useful in onError.

I guess perhaps if you have multiple layers of error handling. Like if you can't log an error to a remote server you might log the log-server-error to the console. Then it would make sense.

... so yeah, I kind of changed my mind mid-post here, but I think I'm with you. error should also be provided to onError

jorgebucaran commented 7 years ago

It seems one of architectural advantages of subscriptions is that they are an array, so my idea is not as flexible unless we make each of the hooks also an array like subscriptions are now. Basically what @dodekeract was saying.

I think I've run out of ideas. 😄

jorgebucaran commented 7 years ago

I've updated the issue's code above to clarify what this proposal is about.

pedroborges commented 7 years ago

After inspecting the core code closely. I notice a few patterns:

1) onAction, onUpdate and onError notify that something happened. They pass some payload and you are not allowed to return anything or change the flow. They are not hooks since you can't really hook into the current process to change it. They are events that can have any number of listeners.

2) onRender is definitely a hook. It passes the model and view and expects a view in return. The call goes from one function then to the next on the stack, each receiving the changed payload from the previous one. It's similar to the concept of middlewares.

`onRender` needs to run before the `onLoad` method is called. This is why it has been moved to its own method. You guys may have a better idea on this.

3) Actions is a thing on it's own. They are good as they are in my opinion.

4) Subscriptions are called when the core finishes loading and is a great place to express your intention to listen to other events from the core and the world outside the app. This conforms to the Elm architecture:

Subscriptions — A Sub lets you register that you are interested in something: tell me about location changes, listen for web socket messages, etc. It has been discussed and I agree that onLoad is a better name for this. The purpose stays the same.

For me it could be just a callback instead of an array since the parameters signature is the same. For an app that needs to listen to many events and hooks these would mean less code.

app({
    actions,
    model,
    view,
    plugins: [Router],

    beforeRender: (model, view) => {
      // do something…
      return view
    },

    onLoad: (model, actions, emitter) => {
        if (model.contacts.length < 1) {
            actions.contacts.addRandom()
        }

        window.addEventListener('popstate', e => {
            actions.popstate()
        })

        emitter.on('actions', (name) => console.log(name))
        emitter.on('update', (model) => console.dir(model))
        emitter.on('error', (msg) => console.error(msg))
    }
})

To achieve this I have adapted the Mitt event emitter micro library and added a new method to handle stacks that need to return a value. This addition allowed me to remove a bunch of duplicate logic resulting in 45 additions and 49 deletions.

Check out the current implementation at: https://github.com/pedroborges/hyperapp/commits/events

This is great because now more events and hooks can be added to Hyperapp making it even more flexible specially allowing for more advanced plugins. The Router can benefit from this immediately as you can see bellow. To add a route:enter hook all we need to do is add one line to the go action. This hook would allow us to check if a user is authenticated, for example:

// router.js
actions: {
  router: {
    match: match,
-   go: function (_, data, actions) {
+   go: function (_, data, actions, emitter) {
+     if (emitter.stack('router:enter', data)) {
        history.pushState({}, "", data)
        actions.router.match(data)
+     }
    }
  }
},

Then on my app's onLoad method I could check and prevent the route from continuing or redirecting to somewhere else:

// my-app.js
emitter.on('route:enter', (route) => {
    if (route.startsWith('/admin') && !model.user) {
        actions.router.go('/login')
    }
})

Since the emitter is in place, now its easy to add a route:leave hook to prevent the user from leaving the page under a condition or doing something after the routing has been completed with route:update, like sending Google Analytics data.

This is concept is called lifecycle hooks and you can check out what other libraries are doing:

If more hooks and events get added, it might be a good idea to namespace them. Then encourage plugins creators to do the same for their events and hooks.

Please notice that my proposal doesn't change how data flows inside the app. It's just an extension to better accommodate how the app interacts with the external world. I'd love to hear your opinion!

jorgebucaran commented 7 years ago

This feature is pretty much done. I just need to fix some tests that are failing.

I am trying to decide which naming style we should use, basically onEventName VS eventName.

(A) events: {
  loaded: (state, actions,                    emit) => {}, ...,
  action: (state, actions, { name, data },    emit) => {}, ...,
  update: (state, actions, data,              emit) => {}, ...,
  render: (state, actions, { match, params }, emit) => {}, ...
  route: (state, actions, { match, params }, emit) => {}, ...
}

(B) events: {
  onLoad: (state, actions,                      emit) => {}, ...,
  onAction: (state, actions, { name, data },    emit) => {}, ...,
  onUpdate: (state, actions, data,              emit) => {}, ...,
  onRender: (state, actions, { match, params }, emit) => {}, ...
  onRoute: (state, actions, { match, params }, emit) => {}, ...
}
FlorianWendelborn commented 7 years ago

@jbucaran Can this be closed?