MithrilJS / mithril.js

A JavaScript Framework for Building Brilliant Applications
https://mithril.js.org
MIT License
14.02k stars 924 forks source link

[rewrite] Component constructor not being called on route :subpath navigation #1323

Closed simov closed 8 years ago

simov commented 8 years ago

I'm probably missing something, but I was expecting the oninit ctor to be called each time I click on a navigation link:

m.route(document.querySelector('body'), '/card/small', {
  '/card/:size': {
    oninit: function (vnode) {
      console.log(vnode.attrs.size)
    },
    view: (vnode) =>
      m('ul', [
        m('li', m('a', {href: '/card/small', oncreate: m.route.link}, 'small')),
        m('li', m('a', {href: '/card/medium', oncreate: m.route.link}, 'medium')),
        m('li', m('a', {href: '/card/large', oncreate: m.route.link}, 'large'))
      ])
  }
})

What happens is that oninit gets called only on initial route navigation. How am I supposed to handle this case? Couldn't find it in the migration guide either.

simov commented 8 years ago

Got it working with this:

m.route(document.querySelector('body'), '/card/small', {
  '/card/:size': {
    oninit: (vnode) => {
      vnode.state.current = vnode.attrs.size
    },
    onupdate: (vnode) => {
      if (vnode.state.current !== vnode.attrs.size) {
        vnode.state.current = vnode.attrs.size
        console.log('path changed to', vnode.attrs.size)
      }
    },
    view: (vnode) =>
      m('ul', [
        m('li', m('a', {href: '/card/small', oncreate: m.route.link}, 'small')),
        m('li', m('a', {href: '/card/medium', oncreate: m.route.link}, 'medium')),
        m('li', m('a', {href: '/card/large', oncreate: m.route.link}, 'large'))
      ])
  }
})

It wasn't immediately clear to me that the component is a vnode that can contain any of the lifecycle methods in it.

Anyway, kind of ugly but it works, no idea if that's the intended way to do it.

simov commented 8 years ago

I actually looked up at the new docs for the first time and found it :) https://github.com/lhorie/mithril.js/blob/rewrite/docs/route.md#advanced-component-resolution

var Index = {
  view: (vnode) =>
    m('ul', [
      m('li', m('a', {href: '/card/small', oncreate: m.route.link}, 'small')),
      m('li', m('a', {href: '/card/medium', oncreate: m.route.link}, 'medium')),
      m('li', m('a', {href: '/card/large', oncreate: m.route.link}, 'large')),
      m('li', vnode.attrs.data)
    ])
}

m.route(document.querySelector('body'), '/card/small', {
  '/card/:size': (() => {
    var current, data
    return {
      onmatch: (resolve, args, requestedPath) => {
        if (current !== args.size) {
          current = args.size
          // ajax
          setTimeout(() => {
            data = args.size // test
            resolve(Index)
          }, 1000)
        }
      },
      render: (vnode) => {
        return m(Index, {data})
      }
    }
  })()
})
pygy commented 8 years ago

Calling resolve() doesn't trigger the oninit hook... If you want oninit to fire when you change size, you must set the vnode.key in render()... Give this a try:

var Index = {
  oninit(vnode) {
    console.log('oninit', vnode.attrs.size)
    setTimeout(()=> m.redraw(), 200) // to get an update as well...
  },
  onupdate(vnode) {
    console.log('onupdate', vnode.attrs.size)    
  },
  view: (vnode) =>
    m('ul', [
      m('li', m('a', {href: '/card/small', oncreate: m.route.link}, 'small')),
      m('li', m('a', {href: '/card/medium', oncreate: m.route.link}, 'medium')),
      m('li', m('a', {href: '/card/large', oncreate: m.route.link}, 'large')),
      m('li', vnode.attrs.size)
    ])
}

m.route(document.querySelector('body'), '/card/small', {
  '/card/:size': {
    onmatch(resolve, args, requestedPath) {
      // ajax
      setTimeout(() => {
        resolve(Index)
      }, 200)
    },
    render(vnode) {
      vnode.key = vnode.attrs.size
      return vnode
    }
  }
})

Assuming this works as you expect (it should behave as your initial description), this is what I meant by "not using [the RouteResolver] API optimally". @mindeavor and @barneycarroll were right it appears to be harder to learn than I would have imagined...

simov commented 8 years ago

Your code doesn't call oninit multiple times either. But there is no need to, it's just that the ajax logic is now moved out of oninit and it's placed inside the route resolver:

'/card/:page': (() => {
  var page, items
  return {
    onmatch: (resolve, args, requestedPath) => {
      if (page !== args.page) {
        page = args.page
        if (page === 'simple') {
          resolve(app.components['card-simple'])
        }
        else if (page === 'github-events') {
          resolve(app.components['card-github-events'])
          m.request({url: 'https://api.github.com/events'})
            .run((body) => (items = body))
            .catch((err) => console.log(err))
        }
        else if (page === 'newsapi') {
          resolve(app.components['card-newsapi'])
          m.request({url: 'https://newsapi.org/v1/articles?source=techcrunch&apiKey='})
            .run((body) => (items = body.articles))
            .catch((err) => console.log(err))
        }
      }
    },
    render: (vnode) => {
      vnode.attrs.items = items
      return vnode
    }
  }
})()

A bit out of context, but hopefully explains in more details what I'm doing.

pygy commented 8 years ago

You're right, and it is a router bug (my fault). Filing a PR...

For now, this will behave as you'd expect:

    render(vnode) {
      vnode.key = vnode.attrs.size
      return [vnode] // Note the array here.
    }
pygy commented 8 years ago

Keep an eye on #1324. Once it is merged, the additional array won't be needed in your code.

simov commented 8 years ago

I don't know anything about the key property, so I'm not sure what the bug is.

simov commented 8 years ago

@pygy if you take a look at my last code snippet, that's exactly what I'm doing and the code looks pretty reasonable to me.

simov commented 8 years ago

That's the component:

app.components['card-github-events'] = {
  oninit: (vnode) => {
    vnode.attrs.items = []
  },
  view: (vnode) =>
    m('.mdl-grid', vnode.attrs.items.map((event) =>
      m('.mdl-card.mdl-shadow--2dp mdl-cell.mdl-cell--4-col m-card', [
        m('.mdl-card__title',
          m('h2.mdl-card__title-text', event.actor.display_login)
        ),
        m('.mdl-card__media'),
        m('.mdl-card__supporting-text',
          'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Mauris sagittis pellentesque lacus eleifend lacinia...'
        ),
        m('.mdl-card__actions.mdl-card--border',
          m('.mdl-button', 'Related Action')
        )
      ])
    ))
}
pygy commented 8 years ago

Yes, indeed, with more context it looks fine (and dandy).