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

[v4] Allow setState to accept an Event object? #528

Closed
gilbert opened this issue Jan 12, 2017 · 10 comments
Closed

[v4] Allow setState to accept an Event object? #528

gilbert opened this issue Jan 12, 2017 · 10 comments
Labels
proposal type:feature A feature request

Comments

@gilbert
Copy link
Contributor

gilbert commented Jan 12, 2017

New Feature

Perhaps related to #125

Description

Allow the signature setState(key, value) to accept an Event object as its value (e), causing setState to extract e.target.value.

Context

Forcing two-way data binding to be explicit is good in general, but for very basic cases it's a chore. For example, take the following component:

<script>
module.exports = {
  onInput(attrs) {
    this.state = { password: '' }
  },
  setPassword(e) {
    this.state.password = e.target.value
  }
}
</script>

<div class="password-picker">
  <p>Your password is ${state.password.length} characters long</p>
  <input type="password" value=state.password on-input('setPassword') />
</div>

For this very basic task, I had to write a setPassword handler to capture user input into state. If I had more than input, I would have to either write more handlers, or write a generic handler that differentiates by some kind of key. In any case, it's something I have to write and think about every time I need to handle user input.

However, if setState were to accept an event as its value, it would allow me to write the following instead:

<script>
module.exports = {
  onInput(attrs) {
    this.state = { password: '' }
  }
}
</script>

<div class="password-picker">
  <p>Your password is ${state.password.length} characters long</p>
  <input type="password" value=state.password on-input('setState', 'password') />
</div>

This avoids the need to write any handler at all, and remains very readable for what it does. For anything that needs something more complicated – such as scrubbing out invalid characters for a phone number input – it's not hard to fall back to the "write your own handler" way of doing it.

Open Questions

  • Is this too "magical" for marko's taste?
  • Would this work for all types of input?

Is this something you're interested in working on?

Yes

@patrick-steele-idem
Copy link
Contributor

patrick-steele-idem commented Jan 12, 2017

Just throwing this out there is a possible alternative:

<script>
    module.exports = {
        handleNameChanged(value) {
            this.state.name = value.toUpperCase();

            if (this.state.name.length > 10) {
                this.state.invalidName = true;
            }
        }
    }
</script>

<input value:bind('name')/>
<input value:bind('name', 'handleNameChanged')>

<!-- OR: -->

<input bind('name')/>
<input bind('name', 'handleNameChanged')>

In theory, this could be implemented in user land (via a combination of a custom compile-time transform and a small runtime) and if it proves to be valuable then we could pull it into the core marko. Thoughts?

@patrick-steele-idem
Copy link
Contributor

@gilbert
Copy link
Contributor Author

gilbert commented Jan 13, 2017

Coming from Mithril.js, I always liked teaching two-way data binding in this manner:

var myData = 'initial'

var myComponent = {
  view: function () {
    return m('input[type=text]', {
      value: myData
      oninput: (e) => { myData = e.currentTarget.value }
    })
  }
}

The value in this is it uses plain DOM concepts (value, oninput, e), and the data flow is very clear (on the input DOM event, assign myData to the event target's current value).

In other words, there's very little magic to it. So far, Marko has also avoided data-flow magic for the most part, and is thus very close to plain HTML and JS; it's precisely why I was able to pick it up so easily.

HTML-based frameworks (including angular & vue) do not have the benefit of being plain JS, which is why those other frameworks add easy, "proprietary" ways to bind data – they want to take the chore out of it. However, I don't think it's the best approach to take for these reasons:

  • The data flow is not clear. When you see something like v-model="x" or ng-model="y", it's not immediately clear what that's doing or where it's coming from. Contrast that with everything else Marko does, where all data originates from state (nice and explicit)

  • It takes away from other core features. Marko already has on-click('myMethod'), a solid data-flow feature connecting JS and the view. It'd be nice if that were the primary way for JS to interact with your view, instead of having an additional syntax that splits common use cases.

I think changing setState (via the proposal) or adding a method of some sort (on-click('set','x') ?) will strike a good balance between lack of boilerplate and lack of magic.

@patrick-steele-idem
Copy link
Contributor

Here are some notes from various discussions:

<script>
    module.exports = {
        onInput() {
            this.state.orders = [
                { id: '0', title: 'Foo', shipped: true },
                { id: '1', title: 'Bar', shipped: false },
            ]            
        },

        handleShippedChanged() {
            this.state['orders.${i}.shipped'] = foo;
        }
    }
</script>

<ul>
    <li for(i, order in orders)>
        <input type='checkbox' checked:bind('orders.${i}.shipped', 'handleShippedChanged')> Shipped?
    </li>
</ul>
<script>
    module.exports = {
        onInput() {
            this.state.orders = [
                { id: '0', title: 'Foo', shipped: true },
                { id: '1', title: 'Bar', shipped: false },
            ]            
        }
        toggleShipped(i, event, input) {
            // Add support for a settting a property chain while maintaining
            // immutability
            this.setState('orders', i, 'shipped', input.checked);

            // Or, the long way
            var orders = this.state.orders = [].concat(this.state.orders);
            var order = orders[i] = Object.assign({}, orders[i]); // Create a clone of the order
            order.shipped = input.checked;
        }
    }
</script>

<ul>
    <li for(i, order in orders)>
        <input type='checkbox' checked=order.shipped on-change('setState', i)> Shipped?
    </li>
</ul>

@gilbert
Copy link
Contributor Author

gilbert commented Jan 14, 2017

I'm starting to really like the enhanced setState argument. I think on-change('setState', `orders.${i}.shipped`) is readable, intuitive, and convenient, assuming it does the "immutable update" that we discussed.

Another reason I think it's important to keep the data bind directions separate is to allow for different kinds of data binding without creating more esoteric language constructs. For example, if you have an <input type="text" value:bind('name') />, is the (DOM -> JS) data binding on the oninput or onchange event? Both are valid events; depending on your use case, you may want one over the other.

@mlrawlings
Copy link
Member

@mindeavor Our thinking was :bind would look for both change and input events. But you're right, I've often had an change event handler on a text input to reformat a phone number after input, so it wasn't changing as the user typed.

The issue with on-change('setState', `orders.${i}.shipped`) is that in this case, we want the value of checked, not value.

We could add a new method: on-change('setStateFromState', `orders.${i}.shipped`, `checked`)

@patrick-steele-idem
Copy link
Contributor

@mlrawlings did you mean setStateFromEvent?

@gilbert
Copy link
Contributor Author

gilbert commented Jan 14, 2017

Ideally setState would be smart about events. If I'm not mistaken, checked is an anomaly; all other input types store their user input within elem.value.

The [non-mutually exclusive] options I see are:

  • Upgrade setState's logic to be something like newValue = e.target.checked || e.target.value when given an Event object value
  • Add a setStateFromEvent for the generic case (maybe one would want to set state on a data- attribute instead of input value)

@gilbert
Copy link
Contributor Author

gilbert commented Feb 25, 2017

Checking in again – that first bullet point would be very handy in small components and demos :)

@DylanPiercey
Copy link
Contributor

Currently, our goal is to simplify the runtime and remove these kinds of special cases. I don't believe this issue will be addressed, unless maybe by two-way binding support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal type:feature A feature request
Projects
None yet
Development

No branches or pull requests

4 participants