update view API to be (state, prev, send)

[yoshuawuyts]

Since v2.1.4 (oops, semver) we're now also passing the oldState down to views. The view api now looks like:

const view = (params, state, send, oldState) => choo.view`
  <main>${params.foo} and ${state.bar}</main>
`

Instead I reckon it might be nicer to have:

const view = (state, oldState, send) => choo.view`
  <main>${state.params.foo} and ${state.bar}</main>
`

This would definitely be a breaking change, but actually not that hard to fix (e.g. we can provide a regex heyyyy). What do people think?

[timwis]

👍 to reordering the params. Would it make sense to put params inside the app namespace like location?

[ahdinosaur]

Since v2.1.4 (oops, semver) we're now also passing the oldState down to views.

context for anyone like me who wasn't aware.

@yoshuawuyts to be honest i'm not quite sold on oldState being necessary in views, but happy to follow along with how choo develops. 😄

[ahdinosaur]

or i'm being silly, a similar project vdux also passes in previous state, as do many other projects.

[yocontra]

// view(context, send)
const view = ({ params, state, oldState }, send) => choo.view`
  <main>${params.foo} and ${state.bar}</main>
`

🙏

[kristoferjoseph]

const view = (state, send) => choo.view`
  <main>${state.params.foo} and ${state.bar}</main>
`

[yoshuawuyts]

@kristoferjoseph but where would the oldState go? Right now I'm leaning towards:

const html = require('choo/html')
const view = (state, prev, send) => html`
  <main>${state.params.foo} and ${state.bar}</main>
`

We could maybe get super smart and do a Function.prototype.length check so that it's passed oldState, but only if it cares about it:

const mainView = (state, send) => html`
  <main>${state.params.foo} and ${state.bar}</main>
`
const diffView = (state, prev, send) => html`
  <div>${prev.foo} become ${state.foo}</div>
`

This would have the downside that omitting send() would make it questionable; but I reckon that's a bit of an edge case:

// prev.foo is undefined
const diffView = (state, prev) => html`
  <div>${prev.foo} become ${state.foo}</div>
`

@contra while in ES6 that API looks quite good, it's not that great when not on board of the babel train. I personally only use a tiny subset of ES5 / ES6 and would rather not rely on object destructuring for this particular API

[kristoferjoseph]

@yoshuawuyts OK I can get behind passing prev as well. Was originally adverse to the idea of even needing prev, but it seems useful for more advanced optimizations like determining if you need to re-render subtrees etc.

I am trying to only write valid JavaScript, which is actually saying I don't want to be forced into writing babelscript anymore. If it doesn't work in any current JavaScript runtime it isn't JavaScript. right?

[yoshuawuyts]

@kristoferjoseph haha, yeah that's the idea - but you reckon we should do the Function.length detection? Mainly curious what people think about that / would it be confusing? I'm not entirely convinced myself yet

[timwis]

@yoshuawuyts Regarding the Function.length checking, I'd worry that it's too mysterious/magical, and newcomers would think something's wrong. An alternative would be to put send as the first parameter - then the consumer could choose to leave off prev if they want to and it won't break anything.

[yoshuawuyts]

@timwis yeah, you're totally right - mentally I've been geared to have data go first and any type of callback last, so we should probs just do (state, prev, send) ✨

[kristoferjoseph]

I am not a fan of using the magical Function.length
Totally a valid use case for this, but three function arguments is chill.

[yocontra]

Just to be clear, with this new API params would be a reserved keyword that couldn't be used in state?

[yoshuawuyts]

@contra very good point; it def should be - so yes.

edit: Do you think that's acceptable? If you've got any reservations I'd def be keen to hear them; want to get this API right hey ✨

edit2: The validation call could be done through assert() meaning for production it would have no overhead when -g unassertify is run

[yocontra]

@yoshuawuyts How would that work with non-object states? (#23)

Not against it, but it might add some extra complexity to save one function param (or destructure).

[timwis]

params would be in the app namespace, like location, so you should be able to use it in other models like you normally would, no?

[yoshuawuyts]

So state would always be an object; #23 refers to having non-objects as keys on the state; e.g. being able to do:

app.model({
  namespace: foo,
  state: [ 'bin', 'bar', 'baz' ]
})
// yields state of: { foo: [ 'bin', 'bar', 'baz' ], params: {} }

The implication of #23 is, however, that non-namespaced state must always be an object that can be merged with the existing state. I think the overhead of having state.params should be manageable this way; both technically and for the mental model.

As per #82 choo would then provide two additions to the state: state.location and state.params.

[yocontra]

@yoshuawuyts Ah sorry, misunderstood where you wanted that to go. LGTM 💯

[yoshuawuyts]

Closing as 3.0 is imminent; wanna make sure all issues are taken care of. Releasing soooon™ ✨