ractivejs / ractive

Next-generation DOM manipulation
http://ractive.js.org
MIT License
5.94k stars 396 forks source link

Computed properties don't create implicit bindings #1278

Closed Rich-Harris closed 10 years ago

Rich-Harris commented 10 years ago

Related to #1228 (and #1257, @martypdx?) - computed properties that depend on data from the parent scope don't initialise correctly: http://jsfiddle.net/rich_harris/wyhxq4vL/

mkrn commented 10 years ago

I have this issue now, on 0.5.6

My components are passing a data object as 2-way binding and computed are calculated on any change to that..(

Do you have an idea for a workaround?

martypdx commented 10 years ago

@mkrn use .observe()? Can you put together a small example?

mkrn commented 10 years ago

Yes I couldn't make a fiddle reproduce because of complexity, but what im seeing is - i have component Form with data complex object in it It has many components
<formControl ... data="{{data}}" currentKeypath="" ... />

Inside those can invoke other formControls passing them data the same way In formControl I have computed fieldValue that is get/set which calculates value based on keypath, formulas, and other parameters and sets to proper keypath in that data.

SO I'm seeing that if I set any parameter in that data, all of the computed in all of the components are triggering recalculate..

martypdx commented 10 years ago

Ah, I think I noticed that too last week but forgot about adding an issue. Thanks for bringing it up, I think we can keep it part of this issue for now.

I think the work around was to use implicit binding and not use an explicit parameter on the component:

<formControl currentKeypath="" ... />

Does that work for you?

mkrn commented 10 years ago

So I was going to ask - if i keep data in form, and not pass it through 2-way binding then all controls should have isolated: false, correct? Would it be faster than passing it around? Does it work in version 0.5.6? In issue/fiddle it "works" with this.update inside oninit() is there a way to do that in earlier version?
If not - when is 0.6 hitting npm?) I would certainly like to keep on trying, thanks for help!

martypdx commented 10 years ago

if i keep data in form, and not pass it through 2-way binding then all controls should have isolated: false, correct?

isolated: false is the default. Which means that a component that cannot find the data in its context will go looking up into the parent.

Would it be faster than passing it around?

It is actually slightly faster because there's no mappings to be created or traversed. Caveat: this was from some setup timings I did a couple months back.

Does it work in version 0.5.6? In issue/fiddle it "works" with this.update inside oninit() is there a way to do that in earlier version?

You should be able to use init to achieve the same thing.

martypdx commented 10 years ago

If not - when is 0.6 hitting npm?)

Soon. Just an issue of some bugs to get out of the way and update docs.

mkrn commented 10 years ago

Ok, Excellent so you think this.update inside init() would make all mappings correct for all computed inside all components down the line? Even formComponent within formComponent referring all the way up to data? I will be trying it out tonight, thanks for help! Good luck with release

martypdx commented 10 years ago

I haven't looked into this issue yet, so I don't have any idea why it needs the .update call yet. But it seems to do the trick here :)

You can do a more targeted updated by just calling it on the computed properties that need to update. Though oddly in this case, just updating anything, even a non-existent property, seems to update computations.

mkrn commented 10 years ago

Hi Marty, I did some tests, and I think there are a few issues I discovered that can add to this bug:

I can test more things, really looking forward to resolving this since my forms rely pretty heavily on computed and 2way binding.

Thanks

mkrn commented 10 years ago

So just to add: this was tested with edge build. With versions before components computed wouldn't even see the data or maybe the sequence was wrong.

It would be nice to fix it for 0.6.0 so I can help with whatever

martypdx commented 10 years ago

@mkrn I figured out the core issue is that the viewmodel, where computed properties live, is created before the virtualDOM fragment which is how upstream parent data is found via traversing up the context of the fragment. Meaning, in the case of computed properties that use parent data values, you get unresolved references, that then need to update when the fragment is created.

While that order is fundamentally correct (you want data to be available on initial fragment creation, otherwise everything would need to immediately update when the viewmodel was created), to get parent data for computed properties (which themselves are at the data root for the component) only requires access to the parent context in the fragment of the parent component.

So that needs to get reworked, then we can see what's causing the overzealous updates.

mkrn commented 10 years ago

And this is why this.update does the trick.. that's right.. so maybe the order should be changed, and it would cause the update like you're saying.

Personally I don't think components need to see parent scope, and then i just passed everything through attributes but that may make 2-way binding across all of them slower perhaps.. But if they can and it's faster then I'd use it.

martypdx commented 10 years ago

Personally I don't think components need to see parent scope, and then i just passed everything through attributes but that may make 2-way binding across all of them slower perhaps.. But if they can and it's faster then I'd use it.

I think the difference is marginal. And if you use isolated: true it skips all the parent stuff anyway. So, for you, sounds the issue is the unnecessary recalculations.

mkrn commented 10 years ago

I'm glad I helped pinpoint that other one... You're right, so with unnecessary recalculations there are few issues:

martypdx commented 10 years ago

@Rich-Harris spot on with the cross over to #1257 fix. Just had to allow resolveRef to proceed and it worked all the magic.

@mkrn created new issue #1293 for unnecessary recalculations.