[Bug] v1.4.0 Computed properties depending on other computed properties not properly resolved

seijikun commented 3 years ago

Description:

Having two computed properties, with one being a simple redirect from another computed property. The second computed property (depending on the other one) does not seem to work properly. Neither is the initial value correct, nor does it ever get updated. This seems to work with the Ractive version distributed at https://cdn.jsdelivr.net/npm/ractive, but not with 1.4.0.

Versions affected:

At least:

1.4.0-build-55
1.4.0-build-56

Reproduction:

Minimal example:

<html>
    <head>
        <title></title>
        <script src="ractive.min.js"></script>
    </head>
    <body>
        <div id="content"></div>
        <script>
            let MyComponent = Ractive.extend({
                data: function() {
                    return {
                        data: {
                            name: ''
                        }
                    };
                },
                computed: {
                    'state.isNameValid': 'typeof(data.name) === "string" && data.name.length > 0',
                    'state.isValid': 'state.isNameValid'
                },
                template: `
                    <input type="text" value="{{data.name}}" required/>
                    <button {{#if !state.isValid}} disabled {{/if}}>Submit</button>
                `
            });

            var instance = new MyComponent({
                el: document.getElementById('content')
            });
        </script>
    </body>
</html>

Interesting: When adding an additional {{state.isNameValid}} somewhere in the template, the state.isValid property suddenly starts to work normally. e.g.:

template: `
    <input type="text" value="{{data.name}}" required/> {{state.isNameValid}}
    <button {{#if !state.isValid}} disabled {{/if}}>Submit</button>
`

evs-chris commented 3 years ago

This is an interesting case of lazy computeds and implicitly invalidating the parent (and thus all of the children) when computing the computed. Changing the computed to also check its dependencies for invalidation before returning the computed value seems to be the most appropriate way to handle this and avoid breaking other things e.g. there's a reason the implicit invalidation of the parent happens. Once Travis finishes Travising, this should be available as build-57.

seijikun commented 3 years ago

I just downloaded 1.4.0-build-57, but the bug does not yet seem to be fixed, unfortunately. The reproduction example does still not work for me (without the modification below).

seijikun commented 3 years ago

I've only had a rough look at the code, so that's a layman's guess, since I don't know if dirty actually works the way I imagine it to do:

When data.name is updated, its dependents are updated in Model::applyValue() line 164. From there, it reaches the directly dependent computed property: state.isNameValid in ComputationChild::handleChange() line 56.

When it reaches this property, it is already marked dirty (so this.dirty === true). My guess is that this is the case, because it's not used in the template itself, so it's never actually evaluated?

Though the property state.isValid, (which depends on state.isNameValid) is used in the template, and is not marked dirty at that point in time. But since state.isNameValid is already marked as dirty, its ComputationChild::handleChange() handler exits early in line 57, without propagating the dirty-flag further to its dependencies. Thus, the dirty flag of state.isValid stays false, even though it should be updated.

Though I don't know why this would work with Ractive 1.3.x then, since at least this part of the code does pretty much the same thing there.

evs-chris commented 3 years ago

This is a particularly chewy chunk of code, with computations being lazy and notifying their parent on change, which in turn, notifies siblings. The latter seems to be the cause of this particular bug, as one computation gets flagged as changed, is called upon to supply a value, and then flagged as changed again in the midst of the value execution. That leaves the dependents thinking they have the most recent value, and the computation thinking it has notified its dependents that they should pull a fresh value.

I've pushed a change that always notifies deps that a change has taken place, even if the computation is already marked dirty, which seems to skirt the data race on the flag by getting the deps to try refreshing the value. Let me know if that fully resolves the issue for you.

seijikun commented 3 years ago

Yes, that fixes it for me now. Thank you very much!

ractivejs / ractive