jorgebucaran / hyperapp

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

Rename appOptions.events to appOptions.on #278

Closed jorgebucaran closed 7 years ago

jorgebucaran commented 7 years ago

Related #277.

zaceno commented 7 years ago

One thing I liked from #277 is the idea of passing the emit function to mixins (as well as returning it from app) Could we still do that, please? As a follow on, we could stop passing the emit in arguments to each ~event~ on and action.

jorgebucaran commented 7 years ago

@zaceno Yeah, definitely that too. I forgot to mention it.

lukejacksonn commented 7 years ago

I'm not entirely convinced on this one.. seems like change for the sakes of change 🤔

What is so wrong with the name events..

At the moment there is app state, actions, events, view all of those words represent things where as the word on really isn't too descriptive.

We are all going to know what it means, but will in confuse new starters more than it will aid us?

That said, sometimes it is confusing explaining to people that these are app events not dom events like onclick.

jorgebucaran commented 7 years ago

We could go back to hooks?

jorgebucaran commented 7 years ago

@lukejacksonn At the moment there is app state, actions, events, view all of those words represent things where as the word on really isn't too descriptive.

Hmm, I guess you are right.

zaceno commented 7 years ago

Personally, the change of 'events' to 'on' is not important to me. In fact it would break things, and I see @lukejacksonn 's point.

But I still do want the stuff I mentioned above :)

SkaterDad commented 7 years ago

I agree with @lukejacksonn and @zaceno that the current naming is good.

Whats the use case for passing the emit function to a mixin outside of the current way through actions?

jorgebucaran commented 7 years ago

@SkaterDad https://github.com/hyperapp/hyperapp/pull/280


@everyone

So it all boils down to this:

app({
  on: { ready: () => alert("OK") }
})

vs

app({
  events: { ready: () => alert("OK") }
})
zaceno commented 7 years ago

@SkaterDad

Now that app returns emit, it makes emit available in the scope of the appOpts.actions and events, so we could make the api cleaner by removing emit from the paramters to actions and events.

... Unless you're defining those functions elsewhere and are merging together the appOptions.

But you could and probably should be merging in actions and events via mixin. So we should pass the emitter to mixins as well.

Beyond just making the API nicer (imo): if you are hacking on patterns for mixing in view-functions, then having the emit passed to your mixin definition means your mixed in views have access to emit, just like the main view does today. And that could enable some neat patterns I imagine.

zaceno commented 7 years ago

But it would break this pattern, if someone was using it:

import moduleA from 'modulea'
import moduleB from 'moduleb'

app({
  state: {
    modulea: moduleA.state,
    moduleb: moduleB.state,
  },
  actions: {
    ...moduleA.actions,
    ...moduleB.actions,
  },
  events: {
    (not exactly sure how people might be mergeing events)
  } 
})

... but that's ~exactly why we have mixins~ a good reason to use mixins. So I see no harm.

jorgebucaran commented 7 years ago

Closing here since:

@lukejacksonn's argument

state, actions, events, view ... represent things ... the word on really isn't too descriptive.

changed my mind.