Infinite batch change in 2.3.20 causes out of order rendering #2323

Closed
ccummings opened this Issue Mar 14, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@ccummings
Contributor

ccummings commented Mar 14, 2016

JSBin: http://jsbin.com/tixanicuwe/1/edit?html,js,output

Prior to 2.3.20, that would output:

First!
Second!

Now, it outputs things in an incorrect order.

Seems to be related to the infinite batching fix

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 14, 2016

Contributor

can you create that w/o components?

Contributor

justinbmeyer commented Mar 14, 2016

can you create that w/o components?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 14, 2016

Contributor

Why is this considered out of order?

I think the outer component should be notified first of the change. This is important if it happens to be removing the inner component, preventing it from seeing the change.

I'm not sure this is a bug.

Contributor

justinbmeyer commented Mar 14, 2016

Why is this considered out of order?

I think the outer component should be notified first of the change. This is important if it happens to be removing the inner component, preventing it from seeing the change.

I'm not sure this is a bug.

@justinbmeyer justinbmeyer self-assigned this Mar 15, 2016

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 15, 2016

Contributor

So, the problem here seems to be that binding updates now happen as part of another batch.

Consider: {foo}="bar".

When bar changes, the update to foo will not be part of the same batch. It will be part of its own batch.

For the most part, this is what you want.

bar might be calculated from a whole host of other observable properties.

Setting foo might kick off updating another host of observable properties.

I have to noodle on this a bit. It might be possible to have bindings pretend they are part of the same batch.

Alternatively, we might need to bring back some of the old depth-first way of doing batching. Maybe at least with when the callbacks get called.

Contributor

justinbmeyer commented Mar 15, 2016

So, the problem here seems to be that binding updates now happen as part of another batch.

Consider: {foo}="bar".

When bar changes, the update to foo will not be part of the same batch. It will be part of its own batch.

For the most part, this is what you want.

bar might be calculated from a whole host of other observable properties.

Setting foo might kick off updating another host of observable properties.

I have to noodle on this a bit. It might be possible to have bindings pretend they are part of the same batch.

Alternatively, we might need to bring back some of the old depth-first way of doing batching. Maybe at least with when the callbacks get called.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Mar 16, 2016

Contributor

I have an update to the batcch.stop function that makes batch callbacks get called after all "synchronous" sub-batches get called:

        stop: function (force, callStart) {
            if (force) {
                transactions = 0;
            } else {
                transactions--;
            }
            if (transactions === 0) {
                collectingBatch = null;
                var batch;
                if(dispatchingBatches === false) {
                    dispatchingBatches = true;
                    var callbacks = [],
                        i;
                    while(batch = batches.shift()) {
                        var events = batch.events;
                        callbacks.push.apply(callbacks,  batch.callbacks );
                        dispatchingBatch = batch;
                        can.batch.batchNum = batch.number;
                        //console.log("dispatching", can.batch.batchNum);
                        var len;

                        if (callStart) {
                            can.batch.start();
                        }
                        for(i = 0, len = events.length; i < len; i++) {
                            can.dispatch.apply(events[i][0],events[i][1]);
                        }

                        can.batch._onDispatchedEvents(batch.number);

                        dispatchingBatch = null;
                        can.batch.batchNum = undefined;

                    }
                    for(i = callbacks.length - 1; i >= 0 ; i--) {
                        callbacks[i]();
                    }
                    dispatchingBatches = false;
                }


            }
        },

update can/util/batch.js with this.

Contributor

justinbmeyer commented Mar 16, 2016

I have an update to the batcch.stop function that makes batch callbacks get called after all "synchronous" sub-batches get called:

        stop: function (force, callStart) {
            if (force) {
                transactions = 0;
            } else {
                transactions--;
            }
            if (transactions === 0) {
                collectingBatch = null;
                var batch;
                if(dispatchingBatches === false) {
                    dispatchingBatches = true;
                    var callbacks = [],
                        i;
                    while(batch = batches.shift()) {
                        var events = batch.events;
                        callbacks.push.apply(callbacks,  batch.callbacks );
                        dispatchingBatch = batch;
                        can.batch.batchNum = batch.number;
                        //console.log("dispatching", can.batch.batchNum);
                        var len;

                        if (callStart) {
                            can.batch.start();
                        }
                        for(i = 0, len = events.length; i < len; i++) {
                            can.dispatch.apply(events[i][0],events[i][1]);
                        }

                        can.batch._onDispatchedEvents(batch.number);

                        dispatchingBatch = null;
                        can.batch.batchNum = undefined;

                    }
                    for(i = callbacks.length - 1; i >= 0 ; i--) {
                        callbacks[i]();
                    }
                    dispatchingBatches = false;
                }


            }
        },

update can/util/batch.js with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment