need to documenting model-routing order?

[krawaller]

When bootstrapping an app in version 2.3.1 we did this:

app.router((route) => [route('/', Submission(app))])

...where Submission is a component that also made an app.model call. This no longer worked in v3, and had to be changed to

const root = Submission(app)
app.router((route) => [route('/', root)])

I'm guessing somehow the new architecture required the app.model calls to have been made before the routing. If this is really so, perhaps it should be documented?

[krawaller]

(extracted from discussion in #130)

[krawaller]

Gaddämmit, apologies - when creating some Webpackbins to figure this out I realise I was fooling myself. It was the changed signature from (params,state,send) to (state,prev,send) that tricked me.

Here's a 2.3.1 version and a 3.0.1 version both running app.router((route) => [route('/', Component(app))]) without problem.

Sorry! :P

[yoshuawuyts]

hahaha, that's all cool ^____^ - glad you managed to solve it! ✨

[krawaller]

Ehm. So. I was confused in my confusion - we DID encounter a bug as initially described. Apologies again, and thank you @mw222rs for setting me straight!

So - this choo v2.3.1 app works fine:

import choo from 'choo'

const Component = app => {
  app.model({
    namespace: 'comp',
    state: {msg: 'Hello!'},
    reducers: {
      goodbye: (data,state)=> ({msg:'Goodbye!'})
    }
  })
  return (params,state,send)=> choo.view`
    <div>
      <button onclick=${e=>send('comp:goodbye')}>Click!</button>
      <p>${state.comp.msg}</p>
    </div>
  `
}

const app = choo()
app.router((route) => [route('/', Component(app))])
document.body.appendChild(app.start())

But if I try to port this to v3.0.1 like this:

import choo from 'choo'
import html from 'choo/html'

const Component = app => {
  app.model({
    namespace: 'comp',
    state: {msg: 'Hello!'},
    reducers: {
      goodbye: (data,state)=> ({msg:'Goodbye!'})
    }
  })
  return (state,prev,send)=> html`
    <div>
      <button onclick=${e=>send('comp:goodbye')}>Click!</button>
      <p>${state.comp.msg}</p>
    </div>
  `
}

const app = choo()
app.router((route) => [route('/', Component(app))])
document.body.appendChild(app.start())

...then I get an error saying action goodbye isn't found.

But, if I change the bootstrapping to this:

const app = choo()
const root = Component(app)
app.router((route) => [route('/', root)])
document.body.appendChild(app.start())

...then it works! So it does seem to matter in v3 that the app.model calls are made before the routing.

[timwis]

Hm, I think the issue is that you can't add models after app.start() is run. Because the view doesn't run app.model() until the route is triggered, which is after app.start(). And my guess is that app.start() calling start() on barracks is what registers the reducers. So you'd somehow have to call start on barracks again.

Are you opposed to registering the model in index.js? Doing so kind of serves as an index for the app.

[krawaller]

It makes sense that app.start shuts the door on registering new models, but since it is a changed behaviour it might warrant a notice, hence the issue.

Absolutely it makes sense to register the model in index.js! Much agree that the vantage point over the state and changes is super-useful, much like Redux.

It doesn't however make sense if you want to make a reusable contained component. This is what we were playing with in our framework composition comparison, and it was while porting our Choo example for that post that we stumbled upon the changed behaviour.

[timwis]

Ah, I see what you mean. My understanding would be that you'd want your component to be what react/redux folks call a "dumb component" - it has no concept of state other than the properties it is passed. In react, that's the difference between this.state and this.props (dumb components only access this.props). They're typically wrapped in a component that handles the state and passes values off to the dumb components.

In choo world, that would be something like:

// submission.js
const ConfirmButton = require('./confirm-button')
module.exports = ({ isConfirming = false, onClickSubmit = () => {}, onClickConfirm = () => {} }) => {
  const input = html`<input>`
  const tree = html`
    <div>
      ${input}
      ${ConfirmButton({ isConfirming, onClickSubmit, () => onClickConfirm(input.value) })}
    </div>`
}

// confirm-button.js
module.exports = ({ isConfirming = false, onClickSubmit = () => {} onClickConfirm = () => {} }) => {
  return isConfirming
    ? html`<button onclick=${onClickConfirm}>Confirm</button>`
    : html`<button onclick=${onClickSubmit}>Submit</button>`
}

// view.js
const Submission = require('./submission')
module.exports = (state, prev, send) => {
  const submission = Submission({
    isConfirming: state.isConfirming,
    onClickSubmit: () => send('setSubmit', true),
    onClickConfirm: (value) => send('confirm', value)
  })
  return html`
    <div>
      ${submission}
    </div>`
}

Caveat: This is practically pseudo-code, I'm writing it very quickly inside this issue, so forgive me if there are any errors in it or if you've already considered it. Hope it helps!

[krawaller]

My understanding would be that you'd want your component to be what react/redux folks call a "dumb component"

Forgive me, but I respectfully disagree! Absolutely we should strive to have dumb components wherever it makes sense, making our app simpler. But in the case of the confirm button, I think that piece of state should ideally be isolated inside the component.

In your example, the only thing I get for free when using the button component is the if-else rendering. I have to track state myself and pass it to the component. That hardly feels worth the indirection cost of using a component in the first place, I'd be better off having the full code in the parent!

I'm humble to the fact that my inexperience with Choo might make me not realise best practices and the idiomatic approach, but from where I stand now I see definite value in being able to package a fully self-contained button component, where the only thing you need to do is to tell it what event to fire upon confirmation and then listen for that event.

Or perhaps the value of having a centralised model trumps the value of isolating concerns in a component? Not an ironic question, I'm genuinely equal parts intrigued and unsure! :)

[dereklieu]

Or perhaps the value of having a centralised model trumps the value of isolating concerns in a component? Not an ironic question, I'm genuinely equal parts intrigued and unsure! :)

I worked on an app that kept component state such as a confirmed = true localized while storing what we thought of as application state, like api query data, in a central redux store.

We found that the bits and pieces we thought of as component state actually added up to a pretty good amount, and usually that component state would affect how other components render. It wasn't terrible in the end, but we're working on a new version with choo that will try to store all the bits and pieces in the central place.

This also gets you an easier path to start the app from a moment in time, as long as you've got a copy of that store handy, which was a requirement for us.

[yoshuawuyts]

@dereklieu the approach @krawaller takes here actually mounts the localized state inside of the global store - that way the logic to declare lil scraps of data (e.g. "does my specific form have input?", "what's the class on my button?") are scoped to the view, whereas stuff that must be accessed from multiple components / views is stored in separate files. Like with everything, this should be done with caution, but I reckon it def makes sense in some cases. And because the "local" scope is just a namespace on the "global" scope, there's no hidden state anywhere. Not too bad I reckon.

@krawaller I def see what you're trying to do; I feel like passing app to child components might be a bit of an anti-pattern; perhaps this would make for a better pattern?:

const app = choo()

app.model(myView.model)
app.router((route) => [ route('/myview', myView) ])

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

/////////////////////////////////////////////////////
// my-view.js
module.exports = myView
myView.model = { /* all of my model */ }

function myView (state, prev, send) {
  /* return my cool view */
}

I reckon there might be cool ways of automating this snippet of boilerplate bulk-require and bulkify. Hope this was useful, hah ✨

[krawaller]

Haha, I was just returning here to say much what @yoshuawuyts did: I think we're missing the point when comparing to React. Yes, the React version of the confirm button component is stateful and breaks hot reloading and time travelling.

But! In the Choo version, even though isolate the definition of the relevant model parts to the component, the actual state is still stored centrally. If you were to inspect the app state at any point, the confButt stuff would be there. My smart component is still dumb!

In essence - Choo is stupidly smart. Put that on the box!

I agree that putting the model definition on the exported function is cleaner than passing app to the constructor, good call.

[dereklieu]

@yoshuawuyts yeah no question, namespacing store properties within a central store is a very smart way to go about this. 👍

[timwis]

Ah, I see what you mean @krawaller - I actually faced a similar dilemma recently when building an editable table component. It lets you select one row at a time, and I wanted a way for that "state" to be localized to the component. I considered using a plain variable but then you lose the benefits described above and break the pattern. I ended with the approach I described above, where it just receives the selected row index as a property, and the consumer of the component can store/update the property however they like. The only thing I'm a bit torn about having a model coupled with the component is that it becomes choo-specific, don't you think? (Right now it uses html/bel and just returns a DOM element, which could theoretically be used without choo)

[yoshuawuyts]

@timwis yeah, you're totally right - in this case using the onload and onunload hooks might be the way to go; could perhaps create a hybrid that works standalone, but if a flag is passed into the constructor it exposes a model that can be pulled into choo - you reckon?

[yoshuawuyts]

Closing because choo@5 will introduce a new API. See #425 for the merged PR. Thanks!