[FEATURE] Adds sync observers back to tracked properties

This PR flushes synchronous observers in the tracked feature flag path during each `notifyPropertyChange`, which gives these observers similar timing semantics to what they currently have without the feature flag enabled. These changes are meant to only affect observers, and while the metadata required is stored with listeners, this PR doesn't add "async listeners" as a concept to the system. The most notable change is the addition of the `suspended` property to active observers. This property is used by setters which are being set (computed property setters, and in the future `dependentKeyCompat` setters), and when observers are triggered to prevent re-entry.
emberjs · May 29, 2019 · 03b0c59 · 03b0c59
1 parent 1cb5f67
commit 03b0c59
Show file tree

Hide file tree

Showing 16 changed files with 237 additions and 107 deletions.
diff --git a/packages/@ember/-internals/environment/lib/env.ts b/packages/@ember/-internals/environment/lib/env.ts
@@ -112,6 +112,20 @@ export const ENV = {
   */
   _JQUERY_INTEGRATION: true,
 
+  /**
+    Whether the app defaults to using async observers.
+
+    This is not intended to be set directly, as the implementation may change in
+    the future. Use `@ember/optional-features` instead.
+
+    @property _JQUERY_INTEGRATION
+    @for EmberENV
+    @type Boolean
+    @default false
+    @private
+  */
+  _DEFAULT_ASYNC_OBSERVERS: false,
+
   /**
     Controls the maximum number of scheduled rerenders without "settling". In general,
     applications should not need to modify this environment variable, but please

diff --git a/packages/@ember/-internals/meta/lib/meta.ts b/packages/@ember/-internals/meta/lib/meta.ts
@@ -76,13 +76,15 @@ interface StringListener {
   target: null;
   method: string;
   kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE;
+  sync: boolean;
 }
 
 interface FunctionListener {
   event: string;
   target: object | null;
   method: Function;
   kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE;
+  sync: boolean;
 }
 
 type Listener = StringListener | FunctionListener;
@@ -528,13 +530,14 @@ export class Meta {
     eventName: string,
     target: object | null,
     method: Function | string,
-    once: boolean
+    once: boolean,
+    sync: boolean
   ) {
     if (DEBUG) {
       counters!.addToListenersCalls++;
     }
 
-    this.pushListener(eventName, target, method, once ? ListenerKind.ONCE : ListenerKind.ADD);
+    this.pushListener(eventName, target, method, once ? ListenerKind.ONCE : ListenerKind.ADD, sync);
   }
 
   removeFromListeners(eventName: string, target: object | null, method: Function | string): void {
@@ -549,7 +552,8 @@ export class Meta {
     event: string,
     target: object | null,
     method: Function | string,
-    kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE
+    kind: ListenerKind.ADD | ListenerKind.ONCE | ListenerKind.REMOVE,
+    sync = false
   ): void {
     let listeners = this.writableListeners();
 
@@ -585,9 +589,11 @@ export class Meta {
         target,
         method,
         kind,
+        sync,
       } as Listener);
     } else {
       let listener = listeners[i];
+
       // If the listener is our own function listener and we are trying to
       // remove it, we want to splice it out entirely so we don't hold onto a
       // reference.
@@ -598,8 +604,20 @@ export class Meta {
       ) {
         listeners.splice(i, 1);
       } else {
+        assert(
+          `You attempted to add an observer for the same method on '${
+            event.split(':')[0]
+          }' twice to ${target} as both sync and async. Observers must be either sync or async, they cannot be both. This is likely a mistake, you should either remove the code that added the observer a second time, or update it to always be sync or async. The method was ${method}.`,
+          !(
+            listener.kind === ListenerKind.ADD &&
+            kind === ListenerKind.ADD &&
+            listener.sync !== sync
+          )
+        );
+
         // update own listener
         listener.kind = kind;
+        listener.sync = sync;
       }
     }
   }
@@ -761,7 +779,7 @@ export class Meta {
             result = [] as any[];
           }
 
-          result.push(listener.event);
+          result.push(listener);
         }
       }
     }

diff --git a/packages/@ember/-internals/metal/index.ts b/packages/@ember/-internals/metal/index.ts
@@ -51,12 +51,7 @@ export { default as getProperties } from './lib/get_properties';
 export { default as setProperties } from './lib/set_properties';
 export { default as expandProperties } from './lib/expand_properties';
 
-export {
-  addObserver,
-  activateObserver,
-  removeObserver,
-  flushInvalidActiveObservers,
-} from './lib/observer';
+export { addObserver, activateObserver, removeObserver, flushAsyncObservers } from './lib/observer';
 export { Mixin, aliasMethod, mixin, observer, applyMixin } from './lib/mixin';
 export { default as inject, DEBUG_INJECTION_FUNCTIONS } from './lib/injected_property';
 export { tagForProperty, tagFor, markObjectAsDirty, UNKNOWN_PROPERTY_TAG } from './lib/tags';

diff --git a/packages/@ember/-internals/metal/lib/chain-tags.ts b/packages/@ember/-internals/metal/lib/chain-tags.ts
@@ -17,7 +17,7 @@ export function finishLazyChains(obj: any, key: string, value: any) {
   }
 
   if (value === null || (typeof value !== 'object' && typeof value !== 'function')) {
-    lazyTags.clear();
+    lazyTags.length = 0;
     return;
   }
 

diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts
@@ -30,6 +30,7 @@ import {
   isClassicDecorator,
 } from './descriptor_map';
 import expandProperties from './expand_properties';
+import { setObserverSuspended } from './observer';
 import { defineProperty } from './properties';
 import { notifyPropertyChange } from './property_events';
 import { set } from './property_set';
@@ -667,7 +668,19 @@ export class ComputedProperty extends ComputedDescriptor {
     let hadCachedValue = cache.has(keyName);
     let cachedValue = cache.get(keyName);
 
-    let ret = this._setter!.call(obj, keyName, value, cachedValue);
+    let ret;
+
+    if (EMBER_METAL_TRACKED_PROPERTIES) {
+      setObserverSuspended(obj, keyName, true);
+
+      try {
+        ret = this._setter!.call(obj, keyName, value, cachedValue);
+      } finally {
+        setObserverSuspended(obj, keyName, false);
+      }
+    } else {
+      ret = this._setter!.call(obj, keyName, value, cachedValue);
+    }
 
     // allows setter to return the same value that is cached already
     if (hadCachedValue && cachedValue === ret) {

diff --git a/packages/@ember/-internals/metal/lib/events.ts b/packages/@ember/-internals/metal/lib/events.ts
@@ -41,7 +41,8 @@ export function addListener(
   eventName: string,
   target: object | Function | null,
   method?: Function | string,
-  once?: boolean
+  once?: boolean,
+  sync = true
 ): void {
   assert(
     'You must pass at least an object and event name to addListener',
@@ -53,7 +54,7 @@ export function addListener(
     target = null;
   }
 
-  metaFor(obj).addToListeners(eventName, target, method!, once === true);
+  metaFor(obj).addToListeners(eventName, target, method!, once === true, sync);
 }
 
 /**

diff --git a/packages/@ember/-internals/metal/lib/mixin.ts b/packages/@ember/-internals/metal/lib/mixin.ts
@@ -35,6 +35,7 @@ import expandProperties from './expand_properties';
 import { classToString, setUnprocessedMixins } from './namespace_search';
 import { addObserver, removeObserver, revalidateObservers } from './observer';
 import { defineProperty } from './properties';
+import { ENV } from '@ember/-internals/environment';
 
 const a_concat = Array.prototype.concat;
 const { isArray } = Array;
@@ -395,20 +396,23 @@ if (ALIAS_METHOD) {
   };
 }
 
-function updateObserversAndListeners(
-  obj: object,
-  key: string,
-  paths: string[] | undefined,
-  updateMethod: (
-    obj: object,
-    path: string,
-    target: object | Function | null,
-    method: string
-  ) => void
-) {
-  if (paths) {
-    for (let i = 0; i < paths.length; i++) {
-      updateMethod(obj, paths[i], null, key);
+function updateObserversAndListeners(obj: object, key: string, fn: Function, add: boolean) {
+  let observers = getObservers(fn);
+  let listeners = getListeners(fn);
+
+  if (observers !== undefined) {
+    let updateObserver = add ? addObserver : removeObserver;
+
+    for (let i = 0; i < observers.paths.length; i++) {
+      updateObserver(obj, observers.paths[i], null, key, observers.sync);
+    }
+  }
+
+  if (listeners !== undefined) {
+    let updateListener = add ? addListener : removeListener;
+
+    for (let i = 0; i < listeners.length; i++) {
+      updateListener(obj, listeners[i], null, key);
     }
   }
 }
@@ -420,13 +424,11 @@ function replaceObserversAndListeners(
   next: Function | null
 ): void {
   if (typeof prev === 'function') {
-    updateObserversAndListeners(obj, key, getObservers(prev), removeObserver);
-    updateObserversAndListeners(obj, key, getListeners(prev), removeListener);
+    updateObserversAndListeners(obj, key, prev, false);
   }
 
   if (typeof next === 'function') {
-    updateObserversAndListeners(obj, key, getObservers(next), addObserver);
-    updateObserversAndListeners(obj, key, getListeners(next), addListener);
+    updateObserversAndListeners(obj, key, next, true);
   }
 }
 
@@ -845,6 +847,8 @@ if (ALIAS_METHOD) {
 // OBSERVER HELPER
 //
 
+type ObserverDefinition = { dependentKeys: string[]; fn: Function; sync: boolean };
+
 /**
   Specify a method that observes property changes.
 
@@ -870,24 +874,48 @@ if (ALIAS_METHOD) {
   @public
   @static
 */
-export function observer(...args: (string | Function)[]) {
-  let func = args.pop();
-  let _paths = args;
+export function observer(...args: (string | Function)[]): Function;
+export function observer(definition: ObserverDefinition): Function;
+export function observer(...args: (string | Function | ObserverDefinition)[]) {
+  let funcOrDef = args.pop();
+
+  assert(
+    'observer must be provided a function or an observer definition',
+    typeof funcOrDef === 'function' || (typeof funcOrDef === 'object' && funcOrDef !== null)
+  );
+
+  let func, dependentKeys, sync;
+
+  if (typeof funcOrDef === 'function') {
+    func = funcOrDef;
+    dependentKeys = args;
+    sync = !ENV._DEFAULT_ASYNC_OBSERVERS;
+  } else {
+    func = (funcOrDef as ObserverDefinition).fn;
+    dependentKeys = (funcOrDef as ObserverDefinition).dependentKeys;
+    sync = (funcOrDef as ObserverDefinition).sync;
+  }
 
   assert('observer called without a function', typeof func === 'function');
   assert(
     'observer called without valid path',
-    _paths.length > 0 && _paths.every(p => typeof p === 'string' && Boolean(p.length))
+    Array.isArray(dependentKeys) &&
+      dependentKeys.length > 0 &&
+      dependentKeys.every(p => typeof p === 'string' && Boolean(p.length))
   );
+  assert('observer called without sync', typeof sync === 'boolean');
 
   let paths: string[] = [];
   let addWatchedProperty = (path: string) => paths.push(path);
 
-  for (let i = 0; i < _paths.length; ++i) {
-    expandProperties(_paths[i] as string, addWatchedProperty);
+  for (let i = 0; i < dependentKeys.length; ++i) {
+    expandProperties(dependentKeys[i] as string, addWatchedProperty);
   }
 
-  setObservers(func as Function, paths);
+  setObservers(func as Function, {
+    paths,
+    sync,
+  });
   return func;
 }