Unnecessary re-rendering in Blaze because with's ReactiveVar uses default equals function about blaze HOT 17 CLOSED

[OT] Can we agree on using ES2015 (ES6) instead of CoffeeScript? ;)

from blaze.

I will try to refrain myself. But you really do not know what you are missing. :-) It is still so much better language.

from blaze.

Yeah honestly I think doing deep data diffing in reactive vars by default will save a lot of rerendering. Then people just need a way to add custom diffing behavior if they want it.

from blaze.

It seems this is the issue we faced in #45 -- please see attached PR

from blaze.

I'm a tabs guy. I feel your pain ;)

from blaze.

My current workaround is:

@currentData = new ReactiveVar null, EJSON.equals
@autorun (computation) =>
  @currentData.set Template.getData()

Probably this is what you meant as well. :-)

from blaze.

I think we should address this in the following way: We add to templates API a method which allows you to configure which equality function you want to use for data context. And this is then used for internal reactive var equality function.

So something like:

template._dataContextEquality(EJSON.equals);

For now I would do it per template, but we could also try to expose some global-level function which changes a default. But I think that would be really hard to implement correctly and assure it continues to work. Personally, I think this per-template should be enough and then code which handles boilerplate (like Blaze Components) can set it for all templates.

from blaze.

Deep data diffing is also costly. Some frameworks are choosing immutability instead. That is also one approach. But for now I think we should make this customizatable and we can change things in Blaze 2.0.

from blaze.

I think it's a less costly default than rerendering, surely?

from blaze.

I think it's a less costly default than rerendering, surely?

I think this is what React is claiming as well. But they diff on rendered output, not on data. We could also do the same. Store internal HTMLJS tree and diff against that (if Blaze is not doing this already), instead of trying to limit reruns.

The downside of EJSON.equals as a default is that it fails on cyclic structures, and some are pretty cyclic (especially if your data context is a complex JavaScript object with prototype and so on, because it went through transform from MiniMongo).

from blaze.

@mitar Thank you for the workaround. Improved my project performance a bit. Run on this issue across one of my project dealing with rendering a tree structure with children as array.
http://stackoverflow.com/questions/43437068/get-equivalent-reactive-functionality-of-template-currentdata-for-specific-dat

Are there any plans to improve this current issue ?

from blaze.

I am using Blaze Components where it is much easier to not data contexts, but variables attached to template instance (component). Then you can configure which equality you want, for example using computed field.

I do not think there is an easy way to change the default there for Blaze 1.0.

from blaze.

@mitar Is there any reason that the following can't be done?

function processProps({ dataVar, storeMap }, currentData) {
    if (_.isObject(currentData)) {
        const propData = _.isObject(dataVar.curValue) ? dataVar.curValue : {};

        for (const [key, val] of Object.entries(currentData)) {
            if (_.has(storeMap, key)) {
                storeMap[key].set(val);
            } else {
                storeMap[key] = new ReactiveVar(val);
                Object.defineProperty(propData, key, {
                    enumerable: true,
                    get: () => storeMap[key].get()
                });
            }
        }

        if (dataVar.curValue !== propData) {
            dataVar.set(propData);
        }
    } else {
        dataVar.set(currentData);
    }
}

Blaze.With = function (data, contentFunc) {
    const view = new Blaze.View('with', contentFunc);
    
    view.dataVar = new ReactiveVar();
    view.storeMap = {};

    view.onViewCreated(() => {
        if (data instanceof Function) {
            // `data` is a reactive function
            view.autorun(() => processProps(view, data()), view.parentView, 'setData');
        } else {
            processProps(view, data);
        }
    });

    return view;
};

Basically all I'm doing here is turning the data context object into a map of getters that route to individual ReactiveVar.get calls for each property. The idea is to make the data context into something similar to how Blaze.Let works. Because the data context dependency never actually changes (unless it is not an object or becomes an object), the view is never re-rendered. Everything still works though because Spacebars lookups and reactive this[prop] accesses link directly to their respective ReactiveVar get calls.

The only downsides I can see to this is are the following:

• this.data[prop] within onCreated/onRendered/onDestroyed is now reactive because it points to the same object of getters.
• Template.currentData() is no longer reactive by itself (and is now redundant because this.data does the same thing).

The first point could be an issue if existing code has this.data[prop] accesses within reactive contexts, because it would cause unexpected invalidations. This could easily be fixed by wrapping in a Tracker.nonreactive block or caching the value beforehand outside of the reactive context. It also has the beneficial side-effect that this.data would now be up-to-date with the current data at access time, rather than being frozen at the initial state.

The second point honestly seems like a benefit unless Template.currentData() is being used inside of a reactive context purely to rerun it whenever any data changes. In my experience, however, the way Template.currentData() currently works causes unnecessary invalidations most of the time because I usually only want one or two keys from the data context. With this change, it would be reactive upon whichever properties you access, which I think would be beneficial in most cases.

Please let me know if I'm overlooking something here. Obviously, this could be a breaking change that users would have to deal with, but it seems worth it.

from blaze.

I don't have currently time to look into this, but the Blaze was made with IE6 support, so I think getters are not available there. We could modernize Blaze and then yes, a lot of this could be done. Vue does something like that. We could even go further (similarly to how Vue is doing) and go to fully proxy mode.

BTW, I think I have seen a package which exposes such getter/setter approach. You might want to try to find it.

from blaze.

@mitar Alright thanks for clarifying; I just wanted to make sure that I wasn't missing anything in my reasoning. I'm actually surprised that Blaze supports IE6, considering that it seems like meteor is only IE9+ since IE8 went EOL (#7761).

from blaze.

Sorry, maybe not IE6, but some older IE definitely. I think Blaze was extracted before those changes in Meteor. In any case, Blaze was not made updated. We could decide to drop support for older versions of IE, but on the other hand a lot of existing users of Blaze are using it because it is what it is. We could be starting modernizing it, but then the question is if we also have resources to add all other features people might want.

from blaze.

I'm closing this issue because it's too old.

If you think this issue is still relevant please open a new one.

Why? We need to pay attention to all the open items so we should keep opened only items where people are involved.

from blaze.