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

add .use() API method #192

Merged
merged 3 commits into from
Jul 24, 2016
Merged

add .use() API method #192

merged 3 commits into from
Jul 24, 2016

Conversation

yoshuawuyts
Copy link
Member

@yoshuawuyts yoshuawuyts commented Jul 21, 2016

Adds the app.use() API method. Relies on yoshuawuyts/barracks#58 to be merged first.

This patch only adds sugar around the existing hooks. It doesn't add new functionality, but makes it so multiple hooks can be registered without leaking details about the hooks API to consumers.

In practice this means that if say you want to add a logger, time travel and hot module reloading utilities, prior to this patch the API would be:

const TimeTravel = require('choo-time-travel')
const Hmr = require('choo-hmr')
const Log = require('choo-log')
const choo = require('choo')

const timeTravel = TimeTravel()
const hmr = hmr()
const log = log()

const app = choo({
  initialState: function () {
    // something similar is needed for hmr, not a thing yet
    return hmr.initialState
  },
  onStateChange: function (data, state, prev, caller, createSend) {
    log.onStateChange(data, state, prev, caller, createSend)
  },
  onAction: function (data, state, name, caller, createSend) {
    log.onAction(data, state, prev, caller, createSend)
    timeTravel.onAction(data, state, prev, caller, createSend)
  },
  onError: function (err, state, createSend) {
    log.onError(err, state, createSend)
  }
})

app.model(timeTravel.model)

const tree = app.start()
document.body.appendChild(tree)

With this patch it would become:

const TimeTravel = require('choo-time-travel')
const hmr = require('choo-hmr')
const log = require('choo-log')
const choo = require('choo')

const timeTravel = TimeTravel()
const app = choo()

app.use(timeTravel)
app.use(hmr())
app.use(log())

app.model(timeTravel.model)

const tree = app.start()
document.body.appendChild(tree)

The goal here is to prevent leaking specifics of the (by necessity) quite complicated hooks API spilling over into consumer land; providing a neat way to append multiple hooks onto the same application.

The alternative approach to .use() would be to provide a separate package that can combine multiple handlers into a single handler (like .use() does under the hood), but I feel it would be so commonly used it'd be cleaner to add it to the API directly and create guides and docs for using it.


But I understand there might be concerns with adding .use(). @shama posted the following in #152 (comment):

Plugins by their nature are convenient and is why every framework implements an API for it. But my goal has been to try something different then what everyone else currently does. I want to see if a client side ecosystem can be built without peer deps.

I feel that peer deps are a legitimate concern, and can definitely become the source of upgrading pains. That said, peer deps do provide a very attractive tradeoff: they remove boilerplate and make it so that application level code becomes front and center, rather than being clouded by the intricacies of hooking up generic functions to framework specific APIs.

I sincerely hope we can get any sort of plugins in the ecosystem to have any sort of complicated core functionality live in a separate package, with a choo specific wrapper available for convenience. I'm not sure what the best approach for this is, but I'm sure we can figure it out and then write it down for others to follow (like we've started to do with designing for reusability).

I hope this makes sense, and y'all can see where I'm coming from for adding .use(). Keen to hear your thoughts! This would be a minor patch btw. Thanks! ✨


Backlog:

Prerequisite for:

@yoshuawuyts yoshuawuyts changed the title .use: add new api method add .use() API method Jul 21, 2016
@timwis
Copy link
Member

timwis commented Jul 21, 2016

This looks a lot easier to use. I'd say 👍 though @shama's point is compelling. I'd love to hear his thoughts on how we could accomplish the above points without plugins...seems like a matter of tradeoffs

@yoshuawuyts yoshuawuyts force-pushed the use branch 2 times, most recently from cb3e315 to 083443d Compare July 21, 2016 22:12
@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 21, 2016

Added a little warning message to both the plugins and API section, I hope it conveys the message clearly enough:

screen shot 2016-07-22 at 00 12 35

@YerkoPalma
Copy link
Member

As far as I can see, if this update is added I could still work with the old way right? I mean passing the hooks directly to the choo() function would still be possible, and the use function is just syntactic sugar then?

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Jul 21, 2016

@YerkoPalma yeah, that's correct - though I reckon in a major patch we might want to remove that - having a single API feels better than having multiple ways of doing things. Up for debate though

@YerkoPalma
Copy link
Member

Then this plugin system is good to me. When we talk about it in the roadmap discussion, I was thinking in more invasive plugin system, bad idea 😄
But this system doesn't introduce dangerous changes, we shouldn't have to be worried about because

  • Doesn't actually change the hook system, just use it
  • If it add peer dependencies, they woudl be added with the actual system anyway, so, again, no big difference

So, IMHO there are more pros than cons in this PR 👍

@yoshuawuyts
Copy link
Member Author

Alright, going to go ahead and merge this as it stops choo being broken because of the bump in barracks8.1.0 (#195). I'm still really keen to hear thoughts on this; as peer dependencies are one of the largest problems with frameworks and we should try and minimize the impact they can have. But I think that this patch smooths out a lot of cool new use cases, and not implementing .use() would cause problems down the road.

Hope everyone can understand; still def wanna keep discussing how to minimize peer dep issues. Cheers! ✨

@yoshuawuyts yoshuawuyts merged commit b650021 into master Jul 24, 2016
@yoshuawuyts yoshuawuyts deleted the use branch July 24, 2016 21:26
@toddself
Copy link
Contributor

I'm not super sure there is an awesome way around peer deps with plugins. Either you have to list a peer dep and hope you don't install incompatible plugins or you don't list the peer dep and hope to hell the user reads the README and they install what they need along side.

@yoshuawuyts
Copy link
Member Author

@toddself aye, yeah peer deps are a sad story - think the best approach is reduce the need for them, and mostly limit them to stuff like tooling, so that even if something breaks at worst it's a bit of tooling that does a sad.

Published this patch as [email protected] ✌️

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