Best pattern to fetch data based on a path

zapaiamarce commented 7 years ago

Expected behavior

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

var app = choo()
app.use(productStore)
app.route('/product/:id', mainView)
app.mount('body')

function mainView (state, emit) {

  return html`  <body onload=${pullData}>
      <h1>product is ${state.product.name}</h1>
    </body>
  `
  function pullData(){
    emitter.emit('fetchProduct');
  }
}

function productStore (state, emitter) {
  emitter.on('fetchProduct', function () {
    fetch('/product/'+state.params.id,function (product) {
       state.product = product;
       emitter.emit('render')
    })
  })
}

I was wondering if this is a good pattern in order to fetch some data when access some URL.

fraserxu commented 7 years ago

@qzapaia I'm wondering this too. Though I have some issue with this approach.

The onload hook seems only get triggered when refreshing the page. But if it was triggered by emit('pushState', '/page/userB') from /page/userA (using same component) and the dom has already "loaded" before, the onload will not be triggered.

I have tried to emit the fetchProduct event inside the mainView function, but the problem is that it will run into an infinite loop if emit('render') is get called in store handler and mainView function will be called all the time.

I have worked with React for a while, and normally we could simply use componentDidMount and that event will be triggered when switch from page to page(or component being mount to view). Not sure if onload is the best solution here though.

yoshuawuyts commented 7 years ago

@qzapaia yeah I think that's quite reasonable

aknuds1 commented 7 years ago

Guys, I think this corresponds to my choo-routehandler framework. Bear in mind it's written for Choo v4, as I haven't got around to looking at v5 yet. However, the core premise of the framework is to facilitate the loading of data required by individual routes, if you see what I mean. So when a route is switched to, it may load data asynchronously if it defines a loadData hook.

I also found on-load inadequate for this sort of thing, so I implemented MutationObserver in choo-routehandler myself, to observe when the rendered route changes. It works well AFAICT.

yoshuawuyts commented 7 years ago

@fraserxu ahh yeah so I think I figured out what's happening in your case: you're recreating the tag on each render, so the onload handler is called on each render. You probably want to apply some form of caching through https://github.com/yoshuawuyts/choo#caching-dom-elements which means a lil bit of boilerplate.

But actually I think using a one-off var might be just as easy:

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

var app = choo()
app.use(productStore)
app.route('/product/:id', mainView)
app.mount('body')

var loaded = false
function mainView (state, emit) {
  if (!loaded) {
    loaded = true
    emitter.emit('fetchProduct');
  }

  return html`
    <body>
      <h1>
        product is ${state.product.name}
      </h1>
    </body>
  `
}

function productStore (state, emitter) {
  emitter.on('fetchProduct', function () {
    fetch('/product/'+state.params.id,function (product) {
       state.product = product;
       emitter.emit('render')
    })
  })
}

fraserxu commented 7 years ago

Hi @yoshuawuyts thanks for the response. I think I've tried all the solutions here.

The issue with using onload is that is called and only called once for the initial rendering, but it's not what I want as I want to be able to fetch data on router change.

The issue with set a loaded flag it kind of the same that it only runs once, but I want it to run on router change.

The issue to emit('fetchProduct') in mainView() will certainly leads to infinite render and fetch loop.

I've setup a demo repo to show what I mean and hopefully it makes more sense with real code. https://github.com/fraserxu/choo-data-fetching-demo

yoshuawuyts commented 7 years ago

@fraserxu oh but detecting route changes shouldn't be that hard - e.g. I think something like this should work:

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

var app = choo()
app.use(productStore)
app.route('/product/:id', mainView)
app.mount('body')

var route = null
function mainView (state, emit) {
  var newRoute = window.location.href
  if (newRoute != route) {
    route = newRoute
    emitter.emit('fetchProduct');
  }

  return html`
    <body>
      <h1>
        product is ${state.product.name}
      </h1>
    </body>
  `
}

function productStore (state, emitter) {
  emitter.on('fetchProduct', function () {
    fetch('/product/'+state.params.id,function (product) {
       state.product = product;
       emitter.emit('render')
    })
  })
}

cideM commented 7 years ago

It does require keeping var route in sync with the current route. I use a helper method for that

const syncRoute = R.curry(
  (
    onChange: (state: StateType, emit: Function) => void,
    viewFunc: ViewFunctionType
  ) => (state, emit) => {
    ifBrowser(() => {
      const newRoute = window.location.href;

      if (state.routing.current !== newRoute) {
        emit(SET_ROUTE, newRoute);
        if (onChange && typeof onChange === "function") onChange(state, emit);
      }
    });
    return viewFunc(state, emit);
  }
);

app.route("*", R.pipe(withLayout, syncRoute(fetchAll)(Main)));

The naming and routing store can probably be improved but that's what I came up with just now. If that's not ideal, someone let me know :O

jonjaques commented 7 years ago

Just throwing my two cents in here :) Feel free to tell me if it's crazy, as either I'm doing it wrong or I may have found a bug with similar problems to #479/#549.

Coming from react land, the event-emitter pattern seems very close to redux actions, so for loading data I would like to use a pattern like so:

function todoStore(state, emitter) {
  const todos = {todo: null}
  state.todos = todos

  emitter.on('navigate', ()=> {
    if (someCondition) {
      emitter.emit('todos.fetchTodo', state.params.id)
    }
  })

  emitter.on('todos.fetchTodo', id => {
    loadTodo(id).then(todo => emitter.emit('todos.fetchTodo.done', todo))
  })

  emitter.on('todos.fetchTodo.done', todo => {
    todos.todo = todo
    emitter.emit('render')
  })
}

By introducing a parameter for todos.fetchTodo event, we can decouple that action from current state and fire it off arbitrarily. From there the listener for fetchTodo.done could cache data or do whatever.

The issue I'm having is that the navigate event is always one behind the current page. In other words, for the first page nav, the handler doesn't fire; then the second nav, what should have fired first then fires. Having trouble figuring that out.

JoeLanglois commented 6 years ago

Is this resolved in a more elegant way? Only thing keeping me from adopting Choo

intellecat commented 6 years ago

This is a very common issue ! Do you really use Choo in your real project ? @yoshuawuyts ~~Forgive my rudeness, but the routing system in Choo really sucks !~~ I'm very rude, and the routing system in Choo really sucks !

marlun commented 6 years ago

@chuck911 Why ask for forgiveness when you're consciously rude? It's not that hard to give constructive feedback. "I see a lot of missing functionality in the routing system in Choo. It would be great if I were able to do X. I could try to create a pull request if someone would be willing to mentor/help me?"

josephluck commented 6 years ago

@chuck911 - You're being very hurtful by saying an open source library that has taken 100s of hours of people's free time sucks. What's more is you haven't suggested anything constructive that solves the issue at hand.

Please take a moment before posting comments that purposefully upset people who are passionate contributors to open source.

intellecat commented 6 years ago

@josephluck @marlun Thank you very much, you are very kind. I was trying to call the author's attention, your replies do help. This issue was posted 8 months ago, the author didn't seem to regard this issue as a real problem. Being nice and polite do nothing here, sometimes someone has to become bad person to tell the truth. It's ok to thumb-down me. I hope this promising framework could be better. @josephluck @marlun Do you gentlemen use this framework usually?

brechtcs commented 6 years ago

I've submitted a PR that might make this easier. Probably not quite ready for merge yet, but could be a starting point...

josephluck commented 6 years ago

@chuck911 - I respectfully disagree, there's already a few ways that the question in this issue can be addressed, and many people have been kind enough to share their experiences in this thread. Demanding the attention of the author in the way you have is pretty disrespectful.

s3ththompson commented 6 years ago

Let's keep this conversation positive. One of the reasons that choo is so great is that it keeps the core minimal by allowing others to come up with the userland solutions that work best for them.

I just published nanofetch which makes it easy to write components that fetch remote data. You may find it useful in solving the above issue with slightly less boilerplate.

choojs / choo

Best pattern to fetch data based on a path #447

Expected behavior