arch-js / arch

Web application framework for React by Red Badger
BSD 3-Clause "New" or "Revised" License
170 stars 13 forks source link

Clarifying the state API #75

Open chrisvfritz opened 9 years ago

chrisvfritz commented 9 years ago

I noticed this one line required a fair bit of explanation in the tutorial:

@props.app-state.get \state.query

I think part of the reason is the state. prefix is passed in as a parameter when it's really a mandatory part of the API for fetching state. It may be useful to have an additional function, get-state, which simply assumes the state. prefix. For example:

@props.app-state.get-state \query

This wouldn't replace the more general purpose get function, but would allow users of the framework to put off learning more about Arch's internals until they're doing non-trivial things with it.

Then there's one other change I'd like to request. I'm not sure if it's too far into Arch's development to change this, but what would you think about renaming app-state to just app?

@props.app.get-state \query

I believe I understand the reason for calling it app-state - it does contain state other than component state. At the same time though, state has become synonymous with component state in the React world, so I fear using the word (even appropriately) to describe other kinds of state will inevitably cause confusion for newcomers to the framework.

What are your thoughts?

chrisvfritz commented 9 years ago

Having now used this interface a little more and with nested cursors (this is also my first time playing with cursors), I don't like the suggestion for the get-state function anymore. However, I'm still not sure why the start function and route components wouldn't just be passed the app-state.get \state sub-trie. Are there cases I'm not thinking of when users of the framework would need to access routing state and other state handled by the framework?

charypar commented 9 years ago

The main reason is the application state is not alone in the app-state hash. There is also the route key the routing updates and when #40 gets done, there will be cookies too.

All of those are potentially useful to a route - writing cookies in response to a user action or a state-based redirect API we've talked about elsewhere (possibly outside Github, in which case I'm sorry). Going indirectly through state and then observing and updateing cookies or route seems unnecessary.

Hope that makes sense.

chrisvfritz commented 9 years ago

That does make sense. Thanks for the explanation! It still bugs me a little that there's a state within app-state that holds a specific kind of state, but the concern it's handling isn't explicitly, no pun intended, stated. :smiley: No obvious alternative is occurring to me though, so I'll close this.

charypar commented 9 years ago

It bugs me too, quite a lot. It generally bugs me that we're dictating the top level schema of the app-state, but the benefits of having these things in there seem to outweigh the drawbacks.

Maybe we could think about a layer of API on top of the basic cursor one which would give aliases to the available things as you suggested. e.g.:

app-state.get-state '...'
app-state.get-cookie '...'
app-state.get-route '...'
tabazevedo commented 9 years ago

So.. potentially when defining an application i.e.

const Application = arch.application.create({
  getInitialState() {
    ...
  },

  routes() {
    ...
  },

  start() {
    // Maybe we can use a react-like API here?
    initObservers(this.state);
  }
});

So... in relevant places you could bind to an application object which would give you this.state, this.cookies and this.route which are currently the cursors in app-state.

Seems like a clean solution for here, and maybe your top-level route components can be passed that object as props so they can do this.props.state / this.props.cookies / this.props.route

charypar commented 9 years ago

@tabazevedo I kinda want to avoid adding too many props to the routes. If it all lives in one app-state, it should just be one prop (also reinforces the idea). But wouldn't mind some API sugar for getting to the relevant parts of app-state.

Not sure I like the idea of an Application global either. I always prefer injection over some global name.

tabazevedo commented 9 years ago

@charypar oh, i didn't mean anything with that application variable. It's just what app.js exports.

charypar commented 9 years ago

@tabazevedo oh sorry, I see, so you're arch = require('arch') and get access to the full state if you need to through arch.application.state?

charypar commented 9 years ago

Wait, no, confusing myself now :) We don't export the application instance from the module...

tabazevedo commented 9 years ago

no, my thought was that when arch calls (your application).start under the hood, instead of passing app-state which is confusing and ambiguous we just bind the this keyword of start to an object

{
  state: app-state.get('state'),
  route: app-state.get('route'),
  cookies: app-state.get('cookies')
}

The object itself isn't stored anywhere really. Maybe we could store it in arch.application but that seems a bit dirty.

charypar commented 9 years ago

Hmm... I don't know if that's helpful. Mostly the place where you want that syntax sugar is route components.

chrisvfritz commented 9 years ago

What's the downside to passing state, cookies, and route as props, instead of app-state? The only one I can think of is users would no longer be able to change the top level of the cursor - which would maybe be something good to enforce.

charypar commented 9 years ago

@chrisvfritz well first it doesn't scale that great - as we add new things we'll have more and more props, second it creates another contract - the props for every single route, not only for things touching app state and finally it hides the (fairly important and useful) fact that all of those things are indeed in the app state, in a single place you can observe (and record history of, instrument, sync with some persistence, etc...).

So even though none of those are particularly bad things (they are sort of "soft" disadvantages), there's quite a few for not too much benefit, which is why I'm reluctant to commit to that idea. Hope that makes sense.

chrisvfritz commented 9 years ago

Thinking on it a little more, access to the entire app-state cursor would be useful for debugging - i.e. allowing users to submit an issue with their entire app state, which should allow users of the framework to immediately reproduce an issue any of their users have. That would be really nice - to put it mildly - so you've convinced me on the value of exposing the entire app state. :smiley:

I think there's something I'm missing though, when you say the individual props idea doesn't scale great. Is there a performance or maintainability hit? If not, and if I'm understanding correctly, those props would only be passed to routes and would always be the only props passed to routes, so there wouldn't be any potential naming conflicts. Could you clarify which problems would be encountered at a larger scale?

And if I'm asking too many questions or they aren't useful, please do feel free to let me know. I'd love to do what I can to help Arch develop and I don't mind being given more direction in that regard (e.g. "We're working on X - don't worry about it. But if you could Y, that'd be great.")

charypar commented 9 years ago

Well I only meant there would potentially be a lot of props we'd pass to a route and it couples the route prop interface to the cursor structure. I meant scale as we add more things to the app-state, not as the app grows.

Asking questions is useful, we should have good answers to them or try and come up with good answers. Plus it's useful for others who may have the same questions but don't ask. :)

chrisvfritz commented 9 years ago

I'm thinking there wouldn't be a maintenance hit to the coupling if we do a merge (slightly more complicated with cursors) of app-state into route props. Then whenever we changed the top-level cursors in app-state, the interface would to the routes would automatically update.

Within the route itself, I don't see there as really being any more coupling. Instead of @props.app-state.get \state, we'd also have the option of just typing @props.state, which one could argue is slightly _de_coupled from the internal structure of app-state.

Though to play devil's advocate again, having a @props.state and @state simultaneously could be disconcerting to people. And it could be a hurdle for folks to remember that all of these incoming props in routes are actually cursors.

Huh, with that in mind, I wonder if all this confusion could be resolved by forgetting all of the above and simply renaming app-state to cursor? Some advantages:

Thoughts?