[3.13] [REGRESSION] Observers are not always triggered

[kanongil]

I have found a critical issue in ember 3.13+, where observers are sometimes not triggered! I have reduced this to a somewhat simple test component:

import Component from '@ember/component';
import { observer, defineProperty } from '@ember/object';

export default Component.extend({
    init() {
        this._super(...arguments);

        this.obj = { a: 0, b: 0 };
        setTimeout(() => this.updateObj(), 1);
    },

    updateObj() {
        this.beginPropertyChanges();
        this.incrementProperty('obj.a');
        this.incrementProperty('obj.b');
        this.endPropertyChanges();
    },

    objAObserver: observer('obj.a', function (target, key) {
        console.log('changed:', key, this.get(key));

        defineProperty(this, 'value', null);
    }),

    objBObserver: observer('obj.b', function (target, key) {
        console.log('changed:', key, this.get(key));
    }),
});

Expected v3.12 behaviour

Inserting the component logs:

changed: – "obj.a" – 1
changed: – "obj.b" – 1

Faulty v3.13 & v3.14.1 behaviour

Inserting the component logs:

changed: – "obj.a" – 1

Bonus info

Removing the begin/endPropertyChanges() restores the 3.12 behaviour, as well as removing the defineProperty() call.

The faulty behaviour seems tied to performing a revalidateObservers() (triggered by the defineProperty()) during a flushSyncObservers().

It is also worth to note that if the observed values are rendered into a template, it is rendered correctly.

Edit: split into two separate observers to more clearly illustrate the issue.

[rwjblue]

Hmmm, isn't the entire point of beginPropertyChanges/endPropertyChanges that we coalesce the change event notifications? If so, I'm not sure why we would actually expect the observer to fire twice.

[rwjblue]

Thanks for updating the issue description, that clarifies my question above.

Note, that I do think that the original issue description scenario (single observer observing multiple properties, both of those properties updated within a beginPropertyChanges/endPropertyChanges) should only fire the observer once.

[kanongil]

While this is unrelated to the deeper issue here, I don't agree that the original case should only fire once. Not based on how I read the docs on observer(), and addObserver().

Additionally, in the current implementation, it is just a shortcut, and it will eventually iterate the path list and call addObserver() for each item:

ember.js/packages/@ember/-internals/metal/lib/mixin.ts

Lines 402 to 406 in 7b9f0af

	let updateObserver = add ? addObserver : removeObserver;
	for (let i = 0; i < observers.paths.length; i++) {
	updateObserver(obj, observers.paths[i], null, key, observers.sync);
	}

[kanongil]

I made another variant of the case, that might better illustrate the seriousness of this issue:

import Component from '@ember/component';
import { observer } from '@ember/object';
import { oneWay } from '@ember/object/computed'

export default Component.extend({
    init() {
        this._super(...arguments);

        this.obj = { a: 0 };
        setTimeout(() => this.updateObj(), 1);
    },

    updateObj() {
        this.set('obj', { a: 1 });
    },

    prop: oneWay('whatever'),

    objAObserver: observer('obj.a', function () {
        console.log('changed:', this.obj.a);

        this.set('prop', true);
    }),

    objObserver: observer('obj', function () {
        console.log('obj observer!')
    }),
});

In this case objObserver doesn't trigger, though it clearly should! Also, I trigger the defineProperty() through the oneWay() property set.

/cc @pzuraq

[kanongil]

I would expect more priority going into fixing an regression that causes observers to not trigger on changes. But maybe this is all part of the scheme to move people away from observers? 😲

[pzuraq]

There's no scheme here, there's just a lot going on and this doesn't seem to be affecting many users. Observers have been considered an antipattern and recommended against for quite some time now, and sync observers in particular will have experienced a major performance regression in 3.13+, so the recommendation is to enable async observers asap, and to migrate off observers asap.

That said, I will take a look at this bug as soon as I have time, it's on the backlog.

[krisselden]

ember.js/packages/@ember/-internals/metal/tests/observer_test.js

Line 837 in d76eac7

[krisselden]

The test for this was removed. Also, this behavior should only apply to sync observers. Basically right now we defer flushing the sync observers, but we need to defer only sending the events and not the flush. It also deduped the send events for the same target/prop.