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

Abstraction of things that call setState #218

Closed
ashaffer opened this issue Aug 4, 2015 · 69 comments
Closed

Abstraction of things that call setState #218

ashaffer opened this issue Aug 4, 2015 · 69 comments

Comments

@ashaffer
Copy link

ashaffer commented Aug 4, 2015

Is there an accepted pattern for doing this? My motivating example is the form below:

function render(component, setState) {
  const {state} = component
  const {dirty, user, submitted} = state

  const {valid, errors} = validateUser(user)
  const errMsgs = userMessages(errors)

  return (
    <form>
      <SignupInput placeholder='FULL NAME' onChange={setField('name')} error={getError('name')} />
      <SignupInput placeholder='EMAIL' onChange={setField('email')} error={getError('email')} />
      <SignupInput type='password' placeholder='PASSWORD' onChange={setField('password')} error={getError('password')} />
    </form>
  )

  function setField (field) {
    return function(element) {
      const value = element.delegateTarget.value
      setState({
        user: assign({}, user, {[field]: value}),
        dirty: assign({}, dirty, {[field]: true})
      })
    }
  }

  function getError (field) {
    return (submitted || dirty[field]) && errMsgs[field]
  }

It is a simple signup form with validation and a model, and i'd like to be able to abstract out setField, getError, and the little bit of validation setup in render, but setField's dependence on the setState function is making things complicated.

The solutions I can think of are:

  • composition around render. e.g. compose(form, render) where form decorates the component param to include something like {state, props, actions: {setField, getError}}.
  • a service that takes setState as an argument, and returns the action functions. E.g. in render const {setField, getError} = form(setState)
  • something more dramatic like using streams/observables as properties on state, and the integration with the setState function this would entail.

The problem with all of them to varying degrees is that they make it less clear where state is being manipulated (which I suppose may just be an inherent tradeoff of abstraction that can't be eliminated).

@anthonyshort
Copy link
Owner

We had that problem with our forms and tried a few different ways of managing the state. Which were basically the three you mentioned there. The way we do it is by storing all the state on the Form component that holds all of the fields and set it like you've done there. It's not ideal but it works for now.

Ideally state would be kept out of the component and we wouldn't need setState but expecting new people to figure that out would be pretty crazy. I'd want to get rid of it completely, but it's hard when simple cases like this would require so much more setup.

If you come up with something simple we should put it into a guide in the docs.

@ashaffer
Copy link
Author

ashaffer commented Aug 4, 2015

@anthonyshort I think the storing of all the state on the form is right. In general I think concentrating state as high up in the tree as possible is the way to go. I thought of two other possible solutions to this problem that seem pretty good to me:

Solution 1: A 2nd order Form component, like so:

Form.js

export default function (validate, messages, Contents) {
  return {
    render() {
      // ... setup validation

      return (<form><Contents setField={setField} getError={getError} /></form>)

      function setField() {
        // ...
      }
      function getError() {
        // ...
      }
  }
}

SignupForm.js

function render(component) {
  const {props, state} = component
  const {setField, getError} = props

  return (
    <Block>
      <SignupInput placeholder='FULL NAME' onChange={setField('name')} error={getError('name')} />
      <SignupInput placeholder='EMAIL' onChange={setField('email')} error={getError('email')} />
      <SignupInput type='password' placeholder='PASSWORD' onChange={setField('password')} error={getError('password')} />
    </Block>
  )
}

export default Form(validateUser, userMessages, {render})

This has the benefit of working with deku as-is and keeping the app-level component stateless.

Solution 2: An actions property on component with methods that are curried by deku to be passed setState.

E.g.

SignupForm.js

import {setField, getError} from 'form'

function render(component) {
  const {props, state, actions} = component
  const {setField, getError, createUser} = actions

  return (
    <form onSubmit={onSubmit}>
      <SignupInput placeholder='FULL NAME' onChange={setField('name')} error={getError('name')} />
      <SignupInput placeholder='EMAIL' onChange={setField('email')} error={getError('email')} />
      <SignupInput type='password' placeholder='PASSWORD' onChange={setField('password')} error={getError('password')} />
      <button type='submit'>Sign Up</button>
    </form>
  )

  function onSubmit() {
    createUser(user)
  }
}

const actions = {
  setField, 
  getError,
  createUser(setState, user) {
    api.createUser(user).then(() => setState({created: true}))
  }    
}

export default {render, actions}

This method is even more explicit about what is happening than the higher-order component, and allows these helper methods to really just be helper methods rather than shoehorning them into the component model, but obviously requires some modifications to deku's core or possibly a custom virtual-element. This method also has the performance benefit of not storing these functions in render's closure, which should be helpful for GC pressure, though i'm not sure exactly how much of a problem that really is.

EDIT: Fixed example 1.

@voronianski
Copy link
Contributor

@ashaffer I really like actions concept 👍 it will be huge! /cc @anthonyshort

@anthonyshort
Copy link
Owner

I like where this is going! Something like this could help remove the need for weird flux architectures and remove the complexity around setState. It would be good to think about all the different use cases and see if this solves many of those problems.

@ashaffer
Copy link
Author

ashaffer commented Aug 5, 2015

Following some sort of 'conservation of state setting locations' principle, you could also then remove setState from render, which would let render really just be in charge of rendering.

A slight issue with this approach is it somewhat restricts the locations where state may be set. In my form currently my state object looks roughly like:

{
  user: {name, email, password},
  dirty: {name, email, password}
}

However, if setField were abstract, probably user would have to be changed to something generic like model and the 'dirtiness' state of each field would have to be packaged into the property, like this:

{
  model: {
    name: {value, dirty},
    email: {value, dirty},
    password: {value, dirty}
  }
}

This also raises the possibility of naming conflicts and other nasty stuff like that. Although you could perhaps mitigate that by doing something like:

const actions = {setField('model')}

Which would also let me go back to using the slightly nicer user property. Also, presumably these actions would need at least setState and state in order to function properly, but what about props?

The props issue could perhaps be addressed by something like an initialActions function?

function initialActions(props, setState) {
  return {
    setField: setField('user', setState),
    onSubmit: function(state) {
      if(state.valid && props.onSubmit)
        props.onSubmit()
    }
  }
}

Which really is just the same thing as initialState except that setState is passed to it. You could even just address this minimally by passing setState to initialState and telling people to put these functions on state itself which would keep deku as clean and minimal as possible. I kind of dislike the idea of diluting the meaning of the term 'state' in this way though, so i'm more inclined towards putting these in an actions object.

@anthonyshort
Copy link
Owner

I'll probably need to sit down and really think about how this will work. But for the dirty flag, we don't really need it for each property. We can just set the instance to dirty an re-render the entire thing. That's what we do at the moment when state changes.

I've been trying to find a way to get setState out of render and the other hooks. It makes things dirty. This setup is reminding me of this blog post about the way Snabbdom does it: https://medium.com/@yelouafi/react-less-virtual-dom-with-snabbdom-functions-everywhere-53b672cb2fe3 We're ending up with a single model and a handler. The only thing I'm not sure about is how this setup would work in a real app with real state.

We'll probably look through our app (which is pretty complex) and see how something like this would make our work easier.

@ashaffer
Copy link
Author

ashaffer commented Aug 5, 2015

@anthonyshort In my case at least, I do actually need an individualized dirty flag for each field. For instance, I don't want to render an 'invalid email address' message if the user hasn't yet entered anything (or tried to submit the form), and I wouldn't want to render it just because they started entering stuff in one of the other fields.

@anthonyshort
Copy link
Owner

Ah, I see, you mean 'dirty' in terms of the form fields. I thought you meant that the state/model had changed :D

@ashaffer
Copy link
Author

ashaffer commented Aug 5, 2015

I really like the idea of having an explicit mapping of behavior to intent, as done by snabbdom in that article, cycle.js and also Raynos/mercury (which is probably the most similar to this actions concept). I also really like the idea of just an update function that accepts an intent and its parameters, possibly with the actions = {...} syntax being a shorthand for building a function with a switch statement?

What's cool about update being a single function in addition to its syntactic cleanliness and implementation simplicity for deku is that you can compose around it. For instance:

function update(setState, intent, ...params) {
  switch(intent) {
    case 'set_value':
      const [field, value] = params
      setState({[field]: value})
    break
    // ...
  }
}

could then be composed around by say, an undo stack that records all of the intents and the new states they produce and allows you to wind forwards or backwards in time easily. Or something that abstractly blocks actions until a loading state resolves, or all kinds of crazy things. You could even eliminate the setState function and just allow update to return a new state (or promise, perhaps) - that would be really functional. Then there would be no such thing as setting state, and the only way to manipulate state would be to send things to your update function. This would also make it easier to abstract things like my setField function, as you wouldn't need to pass around this setState token everywhere. E.g.

function update(intent, ...params) {
  switch(intent) {
    case 'set_value':
      const [field, value] = params
      return setField(field, value)
    break
    // ...
  }
}

function setField(field, value) {
  return {[field]: {dirty: true, value: value}}
}

EDIT: And maybe a render for that component would look something like this?

function render(component, send) {
  const set = setValue(send)

  return (
    <TextInput onChange={set('username')} />
  )
}

const setValue = autocurry(function(send, field, element) {
  const value = element.delegateTarget.value
  send('set_value', field, value)
})

@anthonyshort
Copy link
Owner

Yeah! This all sounds great. I've had thoughts about this sort of thing for a while but I haven't got my head around it completely yet. I think I'm thinking about too many use-cases. It reminds me of Cycle, but it will have a much friendlier API. It's also how Redux pretty much works.

function render ({props, state}, handler) {
  return (
    <TextInput onChange={handler('complete', props.todo)} />
  )
}

export let actions = {
  'complete': function (state, ...params) {

  }
}

But I don't think I fully realize how an app would be structured, with API calls, data being passed down etc. It looks like this architecture would favour having a single model, instead of props and state. You might have a better idea of how this could work than I do at the moment :)

You could even eliminate the setState function and just allow update to return a new state (or promise, perhaps) - that would be really functional.

This would be great. I'm not sure how native event handling fits in here, because it's a side-effect. I'm not sure how pure functional programming would go about handling that.

If you can spec out how this might look, we can get this in.

@ashaffer
Copy link
Author

ashaffer commented Aug 6, 2015

As far as deku is concerned, my proposal is actual incredibly simple. It's just this:

  function renderEntity (entity) {
    var component = entity.component
    var fn = typeof component === 'function' ? component : component.render
    if (!fn) throw new Error('Component needs a render function')
    var result = fn(entity.context, setState(entity))

    if (!result) throw new Error('Render function must return an element.')
    return result
  }

turns into, in the simplest possible implementation, this:

  function renderEntity (entity) {
    var component = entity.component
    var fn = typeof component === 'function' ? component : component.render
    if (!fn) throw new Error('Component needs a render function')
    var result = fn(entity.context, action)
    if (!result) throw new Error('Render function must return an element.')
    return result

    function action(...args) {
      setState(entity, update(context, ...args))
    }
  }

So the two primary functions of a component are now update and render, which are both truly pure functions, in the sense that they accept arguments and return a value, and have no other side-effects (except kind of sort of still the dom event handling, but you could still interpret that as being pure, provided the event handlers don't hold any state themselves).

This enables you to write components like this:

import {setField} from 'form'

function render(component, action) {
  return (<TextInput onChange={handleChange('username')} />)

  function handleChange(field) {
    return function(element) {
      action('set_field', element.delegateTarget.value)
    }
  }
}

function update(component, name, ...args) {
  const {props, state} = component

  switch(name) {
    case 'set_field':
      return {user: setField(state.user, ...args)}
  }
}

export default {update, render}

This allows you to abstract out things like my setField function as just pure functions that take parameters and return a value. It also doesn't force you to use the named action pattern, you can send things to your update function in whatever form you like. You could even create a little helper method to translate an actions object into an update function:

import {setField} from 'form'
import {updateActions} from 'deku-helpers'

const actions = {setField}
export default {render, update: updateActions(actions)}
function updateActions(actions) {
  return function(component, name, ...args) {
    return actions[name](component, ...args)
  }
}

But the important part is that it would leave all that up to the user and keep deku super simple.

As for the idea of merging props and state, from the perspective of the render function you definitely could do that if you made this change. My initial reaction though is that I think the difference between the two is super important to emphasize, because whether or not a component holds any state of its own seems like maybe the most important fact about it. Although, I suppose you could determine that now simply by the presence of the update function, which is even more obvious. There are two problems I see with it:

  1. Property name collisions, which is probably not a huge deal, the component should be aware of the name of its own props.
  2. Is update allowed to set the value of things passed in as props? IMO it shouldn't be, but it seems like a strange rule to enforce, given that they are on the same object. But maybe just making those values constant with respect to update will be fine and not confusing.

EDIT: I just realized that also, since update is just a function, you can decorate it with new actions or modify existing actions by composition as well, which seems like a neat/clean way of doing it. For instance, decorating your update with standard form-related actions could look like this:

export default {
  render
  update: compose(form.update, update)
}

EDIT2: I should add, at the moment, i'm really only thinking about my own use cases (i'm attempting a rewrite of a very large angular SPA with deku, so i'm sure i'll have more soon). Which, thus far in the deku app i'm building are just some buttons and some relatively simple forms. So if you have other use cases in mind that we could come up with solutions for that'd be great, the more use-cases the idea is tested on first the better.

@anthonyshort
Copy link
Owner

We're using deku for our entire app at Segment and the hardest thing we've found with the virtual approach is data flow. I'm wondering how this change could make large app state updates, like modifying a user account or a project, that also includes a request to the server. We have all sorts of complex things happening because we're using app.set.

One way to do this would be to bubble the actions to the top of the component tree and then fire them along the edge of the tree top-down, like DOM events. This means you could pass in something like a project into props (which we can freeze during development so it's actually immutable) and then call the action, and have it mutate the state of a much higher component.

// Some button deep down the chain that removes a project
function render({props, state}, handler) {
  return (
    <button onClick={e => handler('project.remove', props.project)}>Remove Project</button>
  )
}

export default {render}
// The top-level app
function render({props, state}, handler) {
  return <App projects={state.projects} />
}

function update({props, state}, name, ...args) {
  const [project, changes] = args // I'm only half sure you can actually do this :D

  switch (name) {
    case 'project.update':
      return { ...state, projects: api.projects.update(project, changes) }
    case 'project.remove':
      return { ...state, projects: api.projects.remove(project) }
  }
}

export default {update, render}

Then it's up to deku to move events around the tree of components. To make sure you know what actions are available, you could have an actions file that exports symbols or constants for the action names so that you don't get clashes but that would be up to the user.

The concerns you mentioned about merging state/props probably aren't a huge problem, but I do like having that distinction. Naming collisions wouldn't be too much of an issue, the views should have defined models and you can enforce it with the validate hook if you needed to.

With this whole approach we have pure state reducers, (state, actionType, actionData) => newstate, and those updates can be composed pretty nicely.

Couple of things I'm not sure about:

  1. Should actions be able to be stopped while bubbling/capturing? If so, how? Do events need to bubble up or should they just start from the top-down?
  2. Would this make forms less painful? @lancejpollard is working on improving a bunch of ours
  3. Do we still need the concept of context?
  4. Does it make actions too much of a black box? If you name something wrong you could get name clashes and who knows what could happen.
  5. Will this be cleaner than app.set?
  6. Where should API requests live? Can we make these less of a side-effect that can happen in the middle of anywhere?

@joshrtay
Copy link

joshrtay commented Aug 6, 2015

I think bubbling stoppable events would remove the need for context. You could implement bubbling by saying that unhandled actions (update returns undefined) bubble.

@ashaffer
Copy link
Author

ashaffer commented Aug 6, 2015

I was not originally thinking these events would go anywhere but inside the component, but ya this could be a cool way of solving the data-flow problem.

Should actions be able to be stopped while bubbling/capturing? If so, how? Do events need to bubble up or should they just start from the top-down?

Hmm what are you thinking is the use-case for capturing? My reaction is that flowing up would really be the only useful direction for the event to go, and that it should definitely be stoppable, though I think it should be an explicit stop (e.g. e.stopPropagation() or a return false or something) rather than an implicit stop, because its very possible you'd want multiple components in the tree to handle an action. Although maybe you could make propagation opt-in by forcing them to return an empty object?

Would this make forms less painful? @lancejpollard is working on improving a bunch of ours

So, just thinking about my own form problem, I think this would allow you to just import a standard set of form actions into any form component. E.g. 'form.set', 'form.submit'. And your input's could just bubble up a 'form.set' with a field value based on their name. E.g. <input name={name} onChange={handler('form.set', props.name)} />

Do we still need the concept of context?

I think that depends on which part of context you think is important. In your example of the project remove button deep down in the tree, if you wanted to say, render the project's name, e.g. <Button>Remove {props.project.name]</Button>, you'd still need to manually propagate the project down the entire tree, which could be cumbersome. Context would allow you to avoid that problem still. However, i'm not sure that tradeoff is worth it anymore. It seems right to me to pass stuff all the way down, even if it's cumbersome, but that could just be because I haven't built a full-sized app yet :).

Does it make actions too much of a black box? If you name something wrong you could get name clashes and who knows what could happen.

That is definitely the biggest problem with this idea. I'm not sure how to resolve it, or whether we could really resolve it a priori rather than just trying it and seeing how it turns out.

As for the other two questions, I don't have much of an opinion yet. I haven't really worked with app.set or api requests very much yet with deku.

@ashaffer
Copy link
Author

ashaffer commented Aug 6, 2015

In thinking about this some more...i'm thinking that you currently implement your project removal thing something like this?

RemoveButton:

const propTypes = {
  project: {
    source: 'currentProject'
  }
}

function render({props, state}, setState) {
  return (
    <div>
      <button disabled={state.loading} onClick={handleClick}>Remove project</button>
      <div class={state.error ? 'show' : 'hide'}>
        <div class='error'>{state.error}</div>
      </div>
    </div>
  )

  function handleClick() {
    setState({loading: true})
    api.project
      .remove(props.project)
      .then(function() {
        setState({error: null, loading: false})
      }, function (error) {
        setState({error: error, loading: false})
      })
  }
}

api:

export default {
  project: {
    remove (project) {
      http.delete('/project/' + project.id).then(function() {
        app.set('currentProject', null)
    })
  }
}

In the new event-based system, I see how you would handle the deletion of the project. But what about things like a loading-state for buttons or displaying an error if an asynchronous operation fails? How would the button get informed about that?

@ashaffer
Copy link
Author

ashaffer commented Aug 6, 2015

Another thing that is almost certainly necessary is that the update method be able to emit new events up the tree as well, so that update methods can aggregate, combine or otherwise translate lower-level events for the consumption of higher-level components.

Also, crazy idea: Replace all dom event handling with this strategy, so there is only a single unified event abstraction. E.g.

TextInput:

function render () {
  // Trying to assign localFn here would throw an error or something, so that
  // we can ensure events are handled solely by handler()
  return (<input name={props.name} onChange={handler('onChange')} onInput={localFn} />)

  function localFn() { }
}

function update ({props, state}, emit, name, ...args) {
  switch(name) {
    case 'onChange':
      const value = element.delegateTarget.value
      emit('set', props.name, value)
      break
  }
}

And the way higher-level components would listen for bubbling is this:

function render() {
  return (<TextInput name='username' onSet={handler('set')} />)
}

function update({props, state}, emit, name, ...args) {
  switch(name) {
    case 'set':
      // do things
      emit('change')
  }
}

This would solve two problems simultaneously. It solves the naming conflict problem, because each component could have local aliases via the handler method if it wanted to <Form onChange={handler('set_field')} />, and it also makes the actions substantially less magical. Your update does not receive an action unless it's setup with handler on one of the elements in your render.

To maintain the sort of fiction that these are actually pure, these emit/handler functions should be asynchronous, and considered just syntactic sugar for this:

function update(emit) {
  const msgs = []

  // emit is syntactic sugar for this
  msgs.push(['set', field, value])

  return {
    msgs: msgs,
    state: state
  }
}

EDIT: Actually, upon thinking about it further, I think @joshrtay's solution for bubbling is the right one. An event bubbles until it is handled, as determined by causing an update to state. In other words, an event bubbles until it becomes part of someone's state.

This also means you can write components that have no update function themselves, but still use handler() as a way of propagating/translating events upwards. And you can also eliminate that emit function I added above, which was awful.

Deku's entire dataflow model could now be succinctly expressed as 'props flow down, events flow up'.

@ashaffer
Copy link
Author

ashaffer commented Aug 7, 2015

Ok, last repeated post, I swear. But I think i've got it now. If you ignore the thing I said earlier about forcing all event handlers to be handler(), you can do api requests in a neat way:

function render({props, state}, handler) {
  return <button onClick={handleClick}>Remove project</button>

  function handleClick() {
    handler('remove_project_start')
    api.project
      .remove(props.project.id)
      .then(handler('remove_project_success'), handler('remove_project_fail'))
  }
}

And I think the biggest instance of multiple components being interested in a single event will likely be where the local component wants to update its state (in this case, say, a loading bool), and it still wants to notify some unspecified parent. For that, you can just emit two events. A local one and a global one.

So, as I see it the complete concept is:

  • handler() emits events to the update function of the current component
  • update is a function that takes in props/state, and either an event name and argument list, or some sort of event object
  • events bubble up the tree until they cause a state update (truthy return value of update)
  • if a parent component wants to listen for an event, it must explicitly bind it (and alias it) in render, like this: <Thing onSet={handler('set')} />. If a component does not bind to an event, it is silently propagated upwards, sort of like a reverse of context.

Thoughts?

EDIT: Also, how about changing handler to trigger?

@anthonyshort
Copy link
Owner

These are all great ideas. The main thing I'm worried about with the bubbling is that we'd fall into the trap of needing to trace events around to see where state was mutated. I'm going to read more into the Elm architecture to see how they handle it. It makes total sense for local state, and maybe app state can be done elsewhere, maybe even outside of deku.

@ashaffer
Copy link
Author

ashaffer commented Aug 7, 2015

Ya, i'm less certain about the event bubbling. I created an implementation of the whole thing though if you or anyone else wants to play around with it:

https://github.com/ashaffer/deku/tree/update-state

disclaimer: I've only written tests for it, not actually used it yet. So it may be that it is broken in various major ways.

EDIT: I'd also add an argument that i'm only about 70% convinced by: this event bubbling strategy is not actually adding a new abstraction to deku, it's just extending the existing DOM abstraction which the programmer has to contend with anyway. I suppose this is really an argument for saying whichever solution is chosen, it should also probably subsume normal DOM event handling as well.

@joshrtay
Copy link

joshrtay commented Aug 7, 2015

if you have an implicit initialize event, you could get rid of initialState. so a component really is just a render and update.

@ashaffer
Copy link
Author

ashaffer commented Aug 7, 2015

@joshrtay That's true. You can actually replace all of deku's current lifecycle hooks with update calls.

@joshrtay
Copy link

joshrtay commented Aug 7, 2015

In addition or instead of an initalize event you could have a props event or propsUpdate event that receives props. Then update and render just need state.

The problem with it is that it would increase the amount of boilerplate necessary for a component. All components would basically require a trivial update.

function render(state, handler) {

}

function update(state, name, ...args) {
  switch (name) {
    case 'props':
      return { ...state, props: args[0] }
  }
}

@joshrtay
Copy link

joshrtay commented Aug 7, 2015

I also think you may want to consider passing an event as a single object instead of a name and a set of args. So the previous example would become:

function update(state, evt) {
  switch (evt.name) {
    case 'props':
      return { ...state, props: evt.data }
  }
}

Seems a little cleaner to me and has the added benefit allowing for a bubbling bool on the evt that controls bubbling.

@anthonyshort
Copy link
Owner

@joshrtay One minor "downside" to that is that you can't curry the handler to use it nicely within the elements:

<div onClick={handler('completeTodo')}>

You'd need to do something like:

<div onClick={e => handler({ type: 'completeTodo' })}>

but you're right, having an action as a single object like an event feels more correct.

I'm going to try and use this to put together a mini app over the weekend that tries to do some of the more obscure things that we handle in our real app now.

@joshrtay
Copy link

joshrtay commented Aug 8, 2015

hadn't thought of that. that is unfortunate.

also handler should probably be renamed to trigger. you trigger an update.

@joshrtay
Copy link

joshrtay commented Aug 8, 2015

@anthonyshort you could still curry handler/trigger if it's not a direct call to update. right?

function trigger (type, payload) {
  component.update(entity.state, {type: type, payload: payload})
}

@ashaffer
Copy link
Author

ashaffer commented Aug 8, 2015

@joshrtay Doing that does allow currying to possibly be more effective. It seems like there's two plausible options here. I just updated my branch to do the following:

  function triggerEvent (entity) {
    return function () {
      var curried = Array.prototype.slice.apply(arguments)

      return function () {
        var args = curried.concat(Array.prototype.slice.apply(arguments))
        // ...
      }
    }
  }

Which allows you to do things like <TextInput onChange={trigger('set', 'username')} />. But it has the downside of requiring you to do this if you wanted to call trigger outside of there:

<TextInput onChange={handleChange} />

function handleChange(e) {
  trigger('set', 'username', e.delegateTarget.value)()
}

Which is kind of lame. In your approach, you could curry only the first argument, and then pass other parameters in your event object, like: <TextInput onChange={trigger({event: 'set', field: 'username'})} />

With either approach, you could also do neat stuff like

<TextInput onChange={compose(extractValue, trigger('set', 'username'))} />

function extractValue(e) {
  return e.delegateTarget.value
}

@ashaffer
Copy link
Author

ashaffer commented Aug 8, 2015

So i've encountered a slight issue. Not sure what the best approach to solving it is, but I think it's a pretty general problem.

So a form has a validity state, but in theory at least, its validity shouldn't really be a part of your official state object, since it is a computed property of the values currently in the form. So if you don't want to put the validity of the form in your actual state object, then you have to recompute it (and duplicate all the associated code) in both your render and update functions if you want to say, highlight things in red when they are wrong, but also block form submissions when the form is invalid (render highlights, update blocks submissions). Performance wise this is mostly probably negligible, but it isn't very DRY.

It seems like there are a few possible ways to address this:

  • Just let validity be part of the form's state, ignoring that it is computed from other things.
  • Put your validation methods on state. So you do something like state.valid() to check validity.
  • Just deal with it and duplicate the code in update and render, and hope this sort of thing doesn't happen too often.
  • Pass in validity from render to your triggers. E.g. <TextInput onChange={trigger('set', 'username', valid)} />

Anyone have any thoughts on the best of these, or other ideas entirely? I'm kind of leaning towards the last one, at the moment.

EDIT: It occurs to me that most forms have a server-side validation component as well, and that that is an external data source, so validity is probably actually a legitimate component of state. But I think this problem still kind of exists in general.

@ashaffer
Copy link
Author

@anthonyshort I think that's right. Reusable components should never manage their own state, as a rule of thumb. This is probably a good heuristic whether or not deku handles state in its core, but becomes even moreso if it doesn't.

The thing that makes the most sense to me, but ideally wouldn't be prescribed by deku, is that the component tree's state is just one of the properties on the global, singular state atom. E.g. state.ui, and the updates are handled by reducing functions - you could create a neat binding to redux by just composing around virtual-element and combining all your update functions into a single state.ui reducer. But that could totally be an optional 3rd party library.

@anthonyshort
Copy link
Owner

Here's where it's currently up to: https://github.com/dekujs/deku/tree/1.0.0

Removed setState, shouldUpdate, and cleaned up a lot of things. The API is still the same, minus setState but I'm going to see if we can make it backwards compatible. We need to settle on an API soon so we can stop making breaking changes.

@joshrtay
Copy link

Cool. Looking much cleaner. What are you thinking in terms of a global dispatch vs passing dispatch down in props?

Also, have you considered event bubbling instead of a global dispatch? I know you mentioned that you're worried about tracing events around, but I find that when events bubble up they're not too difficult to trace - don't really have a problem with tracing dom events. This would give you the power to decorate events with contextual information and it keeps data flow really simple. Data flows down through props. Data flows up through events. Events that reach the root component can be hooked up to stores.

@ashaffer
Copy link
Author

Looks good, the core is now under 1k SLOC!

+1 on what @joshrtay said about event bubbling. I think an API that looks like render ({props, emit}) where emit just bubbles up the tree, and some higher-level component with direct knowledge of the primary state store (e.g. redux) dispatches messages to be statified. In this way, the entire UI is just another observable generating events that cause state updates.

Assuming that route is taken, the issue that remains is how to tell individual components about the state they're interested in. The approach that the elm/snabbdom people seem to be taking is that you just pass everything down and it's really not as bad as it seems. That seems plausible to me, but maybe we should try to come up with some strategies/patterns for implementing that in vaguely real-world scenarios? Or just some PoC's that attempt to demonstrate that it can be done in a sane way.

@ashaffer
Copy link
Author

For extreme purity and cleanliness, you could not even have a dispatch/emit function, and just let DOM event handlers return new events. E.g.

function render (props) {
  return (
    <div onClick={() => return {type: 'REMOVE_PROJECT'}}>
      Remove Project
    </div>
  )
}

@joshrtay
Copy link

@ashaffer How would listening to custom events work in components higher up the tree? And how would you supply a catchall?

Custom:

render(<App onRemoveProject={removeProject} />)

Catchall:

render(<App onAny={dispatch} />)

@joshrtay
Copy link

@ashaffer Also, how would you stop propagation? Just a stopPropagation bool on the event object?

@ashaffer
Copy link
Author

@joshrtay Your custom example is what I was thinking. Stopping propagation would be weird in the version without emit/dispatch, since the return value of the handler is supposed to be a new event. It might mean that that approach isn't quite right. You could put stopPropagation on the event, but that's not very pure, though maybe that's ok in this case.

As for the catch-all issue, I hadn't thought about that, but I suppose you'd want to be able to generically pipe all events into a state store of some kind. onAny seems fine, or perhaps the top-level component is just an emitter or observable.

@joshrtay
Copy link

I think component.id is still going to be necessary to do a setState like action. Didn't see it in the code base.

function setState (componentId, state) {
  return {type: 'setState', id: componentId, data: state}
}

function render ({id, props}) {
  return (
    <div onClick={() => setState(id, {clicked: true})}>
      Click
    </div>
  )
}

@anthonyshort
Copy link
Owner

Yeah I'm probably going to add setState in as a separate module if possible by letting the user define what the 'handler' function is that is passed as the second param to every render function. This is mostly to make migrating to the new API easier, I think local state is something we probably wouldn't want to encourage.

@joshrtay
Copy link

Sounds good. That keeps it simple.

@ashaffer
Copy link
Author

@joshrtay Ya, I think the argument against using component id's is that that would enforce a de-facto isolated local state. If you don't give components ids, you force the state locality to be expressed in the semantic domain of the application, which allows you to access it from other places if you need to.

Consider what I think is probably a bit of a pathological case for the no-local-state approach: A feed with dropdown menus on each feed item. It seems like the open/closed state should just be local to the dropdown, and that is how almost everyone implements it. But this totally breaks down in at least two pretty common cases:

  • Closing on an external click. You end up doing $('body').on('click', closeAllDropdowns)
  • Restyling parent containers when the dropdown is open. Sometimes you want to shift things over to make space, or do other things like that.

In the local state approach, this requires you to propagate that information around your application in various ugly and complex ways. Without local state, you'd be forced to localize the open/closed bool in some way that is native to your application, which should hopefully make accessing state in remote places a bit more natural. I think, maybe.

@anthonyshort
Copy link
Owner

That's a really great example of how local state breaks things. I'm going to include that in the docs :)

@ashaffer
Copy link
Author

In thinking about it more, the event bubbling concept is probably unnecessary. You can just pass functions down in props to emulate it anyway, and then the linkages are all explicit, and there are no new abstractions.

@anthonyshort
Copy link
Owner

^ Yup!

<App handler={ a => handler('child', a) } />

It's how Elm does it. Now Deku can focus on just rendering those components and calling hooks when needed. Components from React, architecture of Elm, simpler than virtual-dom.

The approach that the elm/snabbdom people seem to be taking is that you just pass everything down and it's really not as bad as it seems. That seems plausible to me, but maybe we should try to come up with some strategies/patterns for implementing that in vaguely real-world scenarios?

Elm handles this by just passing down "context" objects too then you can just keep forwarding on or modifying as you need it. You could just use props for that too. It's not magic like React, but that's probably a good thing. The context would usually only be passed down through the components that are coupled to your app anyway.

I like the idea of finishing this off, pushing 1.0, and providing a lot of solid documentation and guides for these more complex cases people eventually run into.

@ashaffer
Copy link
Author

Ya, context objects passed down in props seems like the right approach to me. You could even do that with the dispatch function too, there's no real reason it needs to be special. <App state={state}, dispatch={dispatch} />.

It seems like the main argument for giving dispatch a privileged place in the API would be so that you could share components that call it. But perhaps its better to say that reusable components just shouldn't be dispatching directly?

If you go that route, it seems to me 1.0 is done :).

As for docs, I would really love to see a curated zoo of corner-cases and difficult real-world problems implemented in elegant ways that the community can learn from and iterate on together. It'd be a fair bit of work, but it's probably something we could all add to as we're developing applications with deku and we come across interesting/instructive patterns.

@ashaffer
Copy link
Author

ashaffer commented Sep 7, 2015

@anthonyshort I think i've encountered a class of things that may be awkward with this approach. Some data is extremely global and used by small components in a way that is essentially impossible to predict from knowledge of their parents.

One example is say, the current window size. Something like a <Figure> component may want that information, but it will be deeply nested in some other components, and propagating that information down through all of the other components seems sort of arbitrary and awkward, because it isn't inherently a component of the parent's state, it seems like.

Another, I have a set of components that render images, and they like to wait until the image is loaded (in the browser's cache) before rendering them. On my global state atom i'd like to have a map of {<url>: <loaded>}, which all of these components may examine to decide if they should render their image yet or not. Since it is impossible for the parent components to know which images the children will want to load in this way, i'd have to propagate the entire map down to all of the children that may or may not render this type of component.

Both of these cases and other things like them seem like maybe they are just inherently global chunks of data? Or can we come up with some pattern for propagating these types of things that doesn't feel so janky?i The thing that springs to mind for me here is just to retain the sources abstraction deku currently has, so that at the top-level you can define a select few pieces of data that are accessible to everyone, say (in my case): state, windowDimensions, and loadedImages, and presumably a few other highly global pieces of data, with the knowledge that these things should be used as sparingly as possible.

@anthonyshort
Copy link
Owner

@ashaffer I was thinking about some of that. Things like Redux and routers might want to put something on a context to pass it all the way down. We could support a single context that the whole tree has access to which would be extremely simple. I'm not sure of the benefits of having nested contexts, so maybe the simple approach would cover most use-cases.

export function render (model, context) {
  return <div></div>
}

You could always just pass a context object down yourself too which would be more explicit and less magic-y.

export function render (model) {
  return <SomeComponent context={model.context} />
}

@ashaffer
Copy link
Author

ashaffer commented Sep 8, 2015

Ya I think a single global context might be the right approach. If components don't have their own state, there is no need for nested contexts anyway. Feels like the right balance of simplicity and power.

@anthonyshort
Copy link
Owner

If components don't have their own state, there is no need for nested contexts anyway.

That makes so much sense! :D Nice.

@troch
Copy link

troch commented Sep 8, 2015

I agree with that. For a while I thought about making my router architecture work with nested routers. Why would you want that if you can nest routes? Same goes with components, why would you want to nest contexts when you can nest stateless components. I don't know if that makes sense 🌁

@ashaffer
Copy link
Author

Is the only thing left to work out how to initialize and update information at the top of the tree?

Also, in a redux like system, most people use bindActionCreators to compose their action creators with dispatch, but that is kind of imperative for the components. Since DOM events are the ultimate sole data source of components, and their return value is currently unused, you could instead have the top-level tree() emit the return values of all DOM event handlers (without bubbling), like so:

E.g.

function render ({actions}) {
  return <div onClick={e => actions.openModal()}>Open</div>
}

// ...

const app = tree(<App/>).pipe(store.dispatch)
store.subscribe(getState => render(app, document.body, {state: getState()})

This would make testing a bit easier, and also make it easy to pipe actions into other sinks, like a global log or whatever.

@DylanPiercey
Copy link
Contributor

@anthonyshort regarding nested contexts; I think they are useful. AFAIK react-router (and other routers) use this kind of context to allow for fancy conveniences like relative routing paths.

It should still be pretty simple even with nested contexts, In one of my libraries I am accomplishing this by providing an explicit "with" function which simply modifies the global context during the invocation of a function and returns the resulting component/nodes. This allows for modifying the context at any level but is also light weight. React originally did something similar to this as well.

Also IMO state helpers inside deku are not needed as libraries like "immstruct" and "baobab" can easily provide this. Not sure if this is the case for everyone, but it seems that whenever I need state that is specific to an individual element/component that often it makes the most sense to simply store that data on the dom directly with some abstraction.

@ashaffer you have mentioned a few times about how critical it is that nodes receive id's. Why is this so important? It would seem to me that if it is an issue with identification of unique nodes to provide state then it would be easy enough to leverage the dom nodes in a map. I am curious how the rest of you are storing your state and how you would handle data that is unique to an individual component?

@joshrtay
Copy link

@anthonyshort What's the status of 1.0?

I like what's happening in 1.0. The render functions of components just receive props. I think that's all deku really needs to do. Virtual element can then be in charge of adding context or state.

@anthonyshort
Copy link
Owner

Well it's about done. But I'm on vacation so I couldn't polish up it and release it :) just need to get some tests passing, but I'm trying to include docs on how to do things like managing state, routing etc.

@qur2
Copy link

qur2 commented Oct 22, 2015

I just read the whole thread and it's very interesting. I do not mean to be pushy, however, being very interested as I'm evaluating tools for an upcoming project, I'd like to know if there is anything coming out soon or not.

Thanks for the good work!

@joshrtay
Copy link

joshrtay commented Nov 1, 2015

@anthonyshort - @ashaffer been working on virtex , a small vdom library which lets you process all vdom side effects with a redux like middleware stack.

You may want to check it out. It looks really cool and you could easily build 1.0 on top of it.

@ashaffer
Copy link
Author

ashaffer commented Nov 1, 2015

That would be neat if you wanted to build it on virtex. I did steal all of deku's tests for virtex, which it mostly passes already, so there's that.

It is designed with a redux middleware stack in mind as the effect processing function, but it isn't really coupled to that, it just allows you to orthogonalize unrelated features in a nice way (e.g. components vs dom ops).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants