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

Recommended view nesting pattern is incompatible with onload #324

Closed
timwis opened this issue Nov 15, 2016 · 5 comments
Closed

Recommended view nesting pattern is incompatible with onload #324

timwis opened this issue Nov 15, 2016 · 5 comments

Comments

@timwis
Copy link
Member

timwis commented Nov 15, 2016

I've recommended and used this pattern in multiple projects, and I'm just realising that it's incompatible with onload :-/ Hopefully it's just early and I'm missing something here...

The below test case illustrates the concept: a "Layout" view acts as a parent view and houses the common container, such as the nav bar. Since the layout is on every page, it's otherwise a good place to house the "fetch all records" action.

What you'll see is that navigating between "Home" and "Add," the console logs "onload" and "fetch" every time, despite the fact that Layout is used for both views. I think the problem has something to do with the fact that index.js (below) executes Layout twice as a function, so bel doesn't know that they are the same onload functions. Er, not really explaining that well, but I think it has something to do with Layout being a function.

  1. Any chance I'm overlooking something or just doing it wrong?
  2. Would it be worth it to try and fix this in order to salvage this pattern?
  3. Is there an alternative pattern for this sort of thing that folks recommend?
const choo = require('choo')
const html = require('choo/html')

const app = choo()

app.model({
  state: {},
  reducers: {
    receive: (data, state) => ({})
  },
  effects: {
    fetch: (data, state, send, done) => {
      console.log('fetch')
      send('receive', done)
    }
  }
})

const Layout = (View) => (state, prev, send) => {
  return html`
    <main onload=${onload}>
      <nav class="nav has-shadow">
        <div class="container">
          <a href="/" class="nav-item is-tab">Home</a>
          <a href="/add" class="nav-item is-tab">Add</a>
        </div>
      </nav>
      ${View(state, prev, send)}
    </main>
  `
  function onload () {
    console.log('onload')
    send('fetch')
  }
}

const Home = (state, prev, send) => html`<div>Home</div>`
const Add = (state, prev, send) => html`<div>Add</div>`
      
app.router((route) => [
  route('/', Layout(Home)),
  route('/add', Layout(Add))
])

const tree = app.start()
document.body.appendChild(tree)
@yoshuawuyts
Copy link
Member

believe you might need cache-element for this to work well - could you give
that a shot?

On Tue, Nov 15, 2016 at 1:37 PM Tim Wisniewski notifications@github.com
wrote:

I've recommended and used this pattern in multiple projects, and I'm just
realising that it's incompatible with onload :-/ Hopefully it's just early
and I'm missing something here...

The below test case illustrates the concept: a "Layout" view acts as a
parent view and houses the common container, such as the nav bar. Since the
layout is on every page, it's otherwise a good place to house the "fetch
all records" action.

What you'll see is that navigating between "Home" and "Add," the console
logs "onload" and "fetch" every time, despite the fact that Layout is
used for both views. I think the problem has something to do with the fact
that index.js (below) executes Layout twice as a function, so bel doesn't
know that they are the same onload functions. Er, not really explaining
that well, but I think it has something to do with Layout being a
function.

  1. Any chance I'm overlooking something or just doing it wrong?
  2. Would it be worth it to try and fix this in order to salvage this
    pattern?
  3. Is there an alternative pattern for this sort of thing that folks
    recommend?

const choo = require('choo')const html = require('choo/html')
const app = choo()
app.model({
state: {},
reducers: {
receive: (data, state) => ({})
},
effects: {
fetch: (data, state, send, done) => {
console.log('fetch')
send('receive', done)
}
}
})
const Layout = (View) => (state, prev, send) => {
return html<main onload=${onload}> <nav class="nav has-shadow"> <div class="container"> <a href="/" class="nav-item is-tab">Home</a> <a href="/add" class="nav-item is-tab">Add</a> </div> </nav> ${View(state, prev, send)} </main>
function onload () {
console.log('onload')
send('fetch')
}
}
const Home = (state, prev, send) => html<div>Home</div>const Add = (state, prev, send) => html<div>Add</div>
app.router((route) => [
route('/', Layout(Home)),
route('/add', Layout(Add))
])
const tree = app.start()document.body.appendChild(tree)


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#324, or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWlepG344w8nyDPo-j6LYTxOfIOIqMDks5q-aejgaJpZM4Kyepm
.

@ahdinosaur
Copy link
Collaborator

@timwis yeah i'd expect that to work as well. you're probably right, it might be because of how bel passes the caller function to on-load which is checked here. so i wonder if it works to call on-load directly and pass in the same "origin", but that's still somewhat of a workaround.

@yoshuawuyts yoshuawuyts added the bug label Jan 7, 2017
@JRJurman
Copy link

JRJurman commented Jan 17, 2017

Hey @timwis, I've just started playing around with Choo for a week, and I also used the same pattern you described for layouts! I've been contemplating another idea though, and it seems to solve your problem (although, I haven't really put it through the grind to see if it falls apart anywhere else).

const choo = require('choo')
const html = require('choo/html')

const app = choo()

app.model({
  state: {},
  reducers: {
    receive: (data, state) => ({})
  },
  effects: {
    fetch: (data, state, send, done) => {
      console.log('fetch')
      send('receive', done)
    }
  }
})

const Layout = (view, state, prev, send) => {
  return html`
    <main onload=${onload}>
      <nav class="nav has-shadow">
        <div class="container">
          <a href="/" class="nav-item is-tab">Home</a>
          <a href="/add" class="nav-item is-tab">Add</a>
        </div>
      </nav>
      ${view(state, prev, send)}
    </main>
  `
  function onload () {
    console.log('onload')
    send('fetch')
  }
}

const Home = (state, prev, send) => html`<div>Home</div>`
const Add = (state, prev, send) => html`<div>Add</div>`

app.router([
  ['/', Layout.bind(this, Home)],
  ['/add', Layout.bind(this, Add)]
])

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

Instead of returning a function that takes in state, prev, send, there is one top-level function, which takes in a view as the first parameter. The view is bound to the function in the router depending on the route.

Side Note, I had to change your router syntax, it was breaking on me.

Either way, I'd love to hear what you think, and if this solves the problem, without introducing it's own! I really liked the pattern you first listed, and I kinda like the consistent method definitions in choo.

@marushkevych
Copy link

@timwis you should use subscriptions instead of onload.

@yoshuawuyts
Copy link
Member

nested onload calls is a super hard problem to solve - there's usually a workaround for nested onload calls tho - feel like we should document this at some point, but closing for now

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

No branches or pull requests

5 participants