marko-js-archive / marko-widgets

[LEGACY] Module to support binding of behavior to rendered UI components rendered on the server or client
http://v3.markojs.com/docs/marko-widgets/
MIT License
141 stars 40 forks source link

Empty arrays trick stateful components into re-renders. #100

Closed maberer closed 8 years ago

maberer commented 8 years ago

I have found that giving an empty array to a state property in the getInitialState() method always tricks stateful widgets into a re-render even if none of the given state properties changed...

Part of a child-component :

...
getInitialState: function (input) {
        return {
             "usefulProp": input.text,
             "completelyUnrelatedProp": [] // tricks the component into re-renders!
        };
}
...

Parent component that uses <app-malicious>

...
<div>bla bla bla</div>
<app-malicious usefulProp="test"/>
...

If the parent (the one that embeds <app-malicious>) re-renders, <app-malicious> always re-renders even if "usefulProp" did not change... --> "completelyUnrelatedProp" tricks the component into a re-render... due to its empty array value...

maberer commented 8 years ago

Help: can easily be reproduced by adding an empty array property to the getInitialState() method of the <app-todo-item/> component of the todoMVC example...

maberer commented 8 years ago

added demo project https://github.com/tindli/marko-widgets-lasso

The <app-rerender2/> component should not render when the button is clicked. But it does. It does not however, if the malicious state property is removed (or replaced with e.g. a number)

maberer commented 8 years ago

The demo project also shows an effect, where an internally used state gets lost on re-rendering... (pseudo state property). Is this intended?

patrick-steele-idem commented 8 years ago

tldr;


Hey @tindli, I finally had some time to look into this. I found that things are working as designed, but you have to be careful with state since only a shallow compare is done when compare the old state and a new state. In your case, your getInitialState() method for the app-rerender2 component is always returning a new empty array:

getInitialState: function(input) {
    return {
        pseudo: false,
        malicious: [] // <-- New array being created here. This will trigger a rerender
    }
}

That new array is triggering a rerender since oldWidget.state.malicious !== newWidget.state.malicious. Similarly:

console.log([] === []); // Output: false

For your second question:

The demo project also shows an effect, where an internally used state gets lost on re-rendering... (pseudo state property). Is this intended?

Yes, that is intended. In your case the getInitialState() is returning a hard coded object that is not based on the input. When a nested widget is given new input we call getInitialState(newInput) to get the state for the new widget. We then compare the new state to the old state to decide if the widget needs to be rerendered. If you don't base the state on the new input then it will reset to the hard coded state.

I feel good that there is no issue here, but please feel free to open another issue if you think we can do a better job describing this behavior in the docs.

maberer commented 8 years ago

@patrick-steele-idem OK thanks for taking a look into this!

I was aware of the shallow comparisons; but I did not expect consequences like that (on arrays that have no content...) but OK the compare shows a difference... MW does what it has to do in case of a difference.

The real takeaway of this issue (at least for me) is the clarification done in "State returned from getInitialState(input) should be based on the input". I do not feel this one is documented in all clarity...

This makes it obvious that state, that is used (and manipulated) (only) somewhere deep in a component hierarchy first has to pass all other components "above" (as state) in order to finally reach the component where it is used "as input". As a consequence, the same stateful data has to be stored multiple times along the component hierarchy... even if the state is only really used somewhere deep down. Now lets assume the state is a lot of text that, combined with the multiple storage places (all intermediate components store the text in state) consumes more memory than necessary...

Additionally, the payload when sending down server generated html could get pretty big again by having the text state stored multiple times as string data... (once for each component in the tree)...

When designing high performance components, they should be stateful (in order to detect skippable components...) - we have described that in our FAQ. On the other hand, stateful widgets consume a lot of memory when the component tree is deep... designing deep component trees is encouraged because we want to build reusable components that are easy to read and thus should be small and focused...

Now I am realizing the importance of the SAV architecture used in todoMVC. Although I am not really a fan of the memory such an architecture consumes, I see that on the component level, state has to be touched by all components down the hierarchy... the only thing we can omit is the centralized store (another place where all the app data is stored additionally)...

We once already discussed about special considerations for state, that is just used to bring it down to the place where it is actually consumed - these data objects could really be stored somewhere centralized... but since components should always work isolated - this concept is not feasible...

It is a bit sad to see that designing with deep component hierarchies can come with pretty high costs - I knew that but I though I can circumvent the effect by having isolated components... that keep their private state even when their public properties change...

I am still not sure why Marko-Widgets resets state properties that are not "public"... (hence not used as input properties)??? What would get lost if it would ignore them...? Because by not placing them as input properties, I declared their value being "private" hence not affected by changes outside the component... why does that get overwritten...?

The "issue" here is primarily for optimizations... I think it is a non-issue for most apps... but as soon as huge stateful properties come into discussion, we might get a problem.

patrick-steele-idem commented 8 years ago

@tindli Storing state in all levels of the component hierarchy is really only a problem on the server since the state must being serialized to be sent to the browser and each component's state will be serialized independently. I've thought about compressing the serialized state using some clever approaches, but we have not had a strong need yet.

In the browser, there really isn't any extra memory overhead since the derived state will just be references to the same in-memory objects. E.g., if 10 different nested components hold a reference to the string then it will just be stored once in memory and the only overhead will be the pointer.

I am still not sure why Marko-Widgets resets state properties that are not "public"... (hence not used as input properties)??? What would get lost if it would ignore them...? Because by not placing them as input properties, I declared their value being "private" hence not affected by changes outside the component... why does that get overwritten...?

If you don't want something to get lost then don't store it in the state. Have it hang off the widget instance instead.

The "issue" here is primarily for optimizations... I think it is a non-issue for most apps... but as soon as huge stateful properties come into discussion, we might get a problem.

For some applications is might make sense to only store the state in the top-level UI component and pass everything else down as input properties (not storing it as state). If you do that you will probably want to implement the shouldUpdate(newInput, state) method for every component that is expensive to rerender. By the way, React has the exact same issue. Most React apps choose to only make the top-level UI component stateful. However, I lean towards making UI components stateful so that don't needlessly rerender and I don't have to waste time implementing the shouldUpdate(input, state) method.

maberer commented 8 years ago

Thanks for clarification.

There is an issue that discusses further ideas on development: The intelligent object store that keeps references on serialization/de-serialization has aroused by this issue: https://github.com/marko-js/marko-widgets/issues/105

This should reduce html weight caused by storing the same data multiple times (per component basis).