Merge pull request #7660 from apollographql/backport-symbol-species-fix

Backport v3.4 Symbol.species fix for Concast and ObservableQuery.

CHANGELOG.md

@@ -8,6 +8,9 @@
 - Reactivate forgotten reactive variables whenever `InMemoryCache` acquires its first watcher. <br/>
   [@benjamn](https://github.com/benjamn) in [#7657](https://github.com/apollographql/apollo-client/pull/7657)
 
+- Backport `Symbol.species` fix for `Concast` and `ObservableQuery` from [`release-3.4`](https://github.com/apollographql/apollo-client/pull/7399), fixing subscriptions in React Native Android when the Hermes JavaScript engine is enabled (among other benefits). <br/>
+  [@benjamn](https://github.com/benjamn) in [#7403](https://github.com/apollographql/apollo-client/pull/7403) and [#7660](https://github.com/apollographql/apollo-client/pull/7660)
+
 ## Apollo Client 3.3.7
 
 ### Bug Fixes

package.json

@@ -56,7 +56,7 @@
     {
       "name": "apollo-client",
       "path": "./dist/apollo-client.cjs.min.js",
-      "maxSize": "25.5 kB"
+      "maxSize": "25.6 kB"
     }
   ],
   "peerDependencies": {

src/__tests__/__snapshots__/exports.ts.snap

@@ -326,6 +326,7 @@ Array [
   "compact",
   "concatPagination",
   "createFragmentMap",
+  "fixObservableSubclass",
   "getDefaultValues",
   "getDirectiveNames",
   "getFragmentDefinition",

src/core/ObservableQuery.ts

@@ -10,6 +10,7 @@ import {
   ObservableSubscription,
   iterateObserversSafely,
   isNonEmptyArray,
+  fixObservableSubclass,
 } from '../utilities';
 import { ApolloError } from '../errors';
 import { QueryManager } from './QueryManager';
@@ -649,6 +650,10 @@ once, rather than every time you call fetchMore.`);
   }
 }
 
+// Necessary because the ObservableQuery constructor has a different
+// signature than the Observable constructor.
+fixObservableSubclass(ObservableQuery);
+
 function defaultSubscriptionObserverErrorCallback(error: ApolloError) {
   invariant.error('Unhandled error', error.message, error.stack);
 }

src/core/__tests__/ObservableQuery.ts

@@ -1920,4 +1920,45 @@ describe('ObservableQuery', () => {
       });
     });
   });
+
+  itAsync("ObservableQuery#map respects Symbol.species", (resolve, reject) => {
+    const observable = mockWatchQuery(reject, {
+      request: { query, variables },
+      result: { data: dataOne },
+    });
+    expect(observable).toBeInstanceOf(Observable);
+    expect(observable).toBeInstanceOf(ObservableQuery);
+
+    const mapped = observable.map(result => {
+      expect(result).toEqual({
+        loading: false,
+        networkStatus: NetworkStatus.ready,
+        data: dataOne,
+      });
+      return {
+        ...result,
+        data: { mapped: true },
+      };
+    });
+    expect(mapped).toBeInstanceOf(Observable);
+    expect(mapped).not.toBeInstanceOf(ObservableQuery);
+
+    const sub = mapped.subscribe({
+      next(result) {
+        sub.unsubscribe();
+        try {
+          expect(result).toEqual({
+            loading: false,
+            networkStatus: NetworkStatus.ready,
+            data: { mapped: true },
+          });
+        } catch (error) {
+          reject(error);
+          return;
+        }
+        resolve();
+      },
+      error: reject,
+    });
+  });
 });

src/utilities/index.ts

@@ -81,6 +81,7 @@ export * from './common/maybeDeepFreeze';
 export * from './observables/iteration';
 export * from './observables/asyncMap';
 export * from './observables/Concast';
+export * from './observables/subclassing';
 export * from './common/arrays';
 export * from './common/errorHandling';
 export * from './common/canUse';

src/utilities/observables/Concast.ts

@@ -1,5 +1,6 @@
-import { Observable, Observer, ObservableSubscription } from "./Observable";
+import { Observable, Observer, ObservableSubscription, Subscriber } from "./Observable";
 import { iterateObserversSafely } from "./iteration";
+import { fixObservableSubclass } from "./subclassing";
 
 type MaybeAsync<T> = T | PromiseLike<T>;
 
@@ -55,7 +56,7 @@ export class Concast<T> extends Observable<T> {
 
   // Not only can the individual elements of the iterable be promises, but
   // also the iterable itself can be wrapped in a promise.
-  constructor(sources: MaybeAsync<ConcastSourcesIterable<T>>) {
+  constructor(sources: MaybeAsync<ConcastSourcesIterable<T>> | Subscriber<T>) {
     super(observer => {
       this.addObserver(observer);
       return () => this.removeObserver(observer);
@@ -66,6 +67,13 @@ export class Concast<T> extends Observable<T> {
     // the results through the normal observable API.
     this.promise.catch(_ => {});
 
+    // If someone accidentally tries to create a Concast using a subscriber
+    // function, recover by creating an Observable from that subscriber and
+    // using it as the source.
+    if (typeof sources === "function") {
+      sources = [new Observable(sources)];
+    }
+
     if (isPromiseLike(sources)) {
       sources.then(
         iterable => this.start(iterable),
@@ -143,7 +151,7 @@ export class Concast<T> extends Observable<T> {
   // Any Concast object can be trivially converted to a Promise, without
   // having to create a new wrapper Observable. This promise provides an
   // easy way to observe the final state of the Concast.
-  private resolve: (result?: T) => void;
+  private resolve: (result?: T | PromiseLike<T>) => void;
   private reject: (reason: any) => void;
   public readonly promise = new Promise<T>((resolve, reject) => {
     this.resolve = resolve;
@@ -238,16 +246,6 @@ export class Concast<T> extends Observable<T> {
   }
 }
 
-// Generic implementations of Observable.prototype methods like map and
-// filter need to know how to create a new Observable from a Concast.
-// Those methods assume (perhaps unwisely?) that they can call the
-// subtype's constructor with an observer registration function, but the
-// Concast constructor uses a different signature. Defining this
-// Symbol.species getter function on the Concast constructor function is
-// a hint to generic Observable code to use the default constructor
-// instead of trying to do `new Concast(observer => ...)`.
-if (typeof Symbol === "function" && Symbol.species) {
-  Object.defineProperty(Concast, Symbol.species, {
-    value: Observable,
-  });
-}
+// Necessary because the Concast constructor has a different signature
+// than the Observable constructor.
+fixObservableSubclass(Concast);

src/utilities/observables/Observable.ts

@@ -6,6 +6,7 @@ import 'symbol-observable';
 
 export type ObservableSubscription = ZenObservable.Subscription;
 export type Observer<T> = ZenObservable.Observer<T>;
+export type Subscriber<T> = ZenObservable.Subscriber<T>;
 
 // Use global module augmentation to add RxJS interop functionality. By
 // using this approach (instead of subclassing `Observable` and adding an

src/utilities/observables/__tests__/subclassing.ts

@@ -0,0 +1,45 @@
+import { Observable } from "../Observable";
+import { Concast } from "../Concast";
+
+function toArrayPromise<T>(observable: Observable<T>): Promise<T[]> {
+  return new Promise<T[]>((resolve, reject) => {
+    const values: T[] = [];
+    observable.subscribe({
+      next(value) {
+        values.push(value);
+      },
+      error: reject,
+      complete() {
+        resolve(values);
+      },
+    });
+  });
+}
+
+describe("Observable subclassing", () => {
+  it("Symbol.species is defined for Concast subclass", () => {
+    const concast = new Concast([
+      Observable.of(1, 2, 3),
+      Observable.of(4, 5),
+    ]);
+    expect(concast).toBeInstanceOf(Concast);
+
+    const mapped = concast.map(n => n * 2);
+    expect(mapped).toBeInstanceOf(Observable);
+    expect(mapped).not.toBeInstanceOf(Concast);
+
+    return toArrayPromise(mapped).then(doubles => {
+      expect(doubles).toEqual([2, 4, 6, 8, 10]);
+    });
+  });
+
+  it("Inherited Concast.of static method returns a Concast", () => {
+    const concast = Concast.of("asdf", "qwer", "zxcv");
+    expect(concast).toBeInstanceOf(Observable);
+    expect(concast).toBeInstanceOf(Concast);
+
+    return toArrayPromise(concast).then(values => {
+      expect(values).toEqual(["asdf", "qwer", "zxcv"]);
+    });
+  });
+});

src/utilities/observables/subclassing.ts

@@ -0,0 +1,28 @@
+import { Observable } from "./Observable";
+
+// Generic implementations of Observable.prototype methods like map and
+// filter need to know how to create a new Observable from an Observable
+// subclass (like Concast or ObservableQuery). Those methods assume
+// (perhaps unwisely?) that they can call the subtype's constructor with a
+// Subscriber function, even though the subclass constructor might expect
+// different parameters. Defining this static Symbol.species property on
+// the subclass is a hint to generic Observable code to use the default
+// constructor instead of trying to do `new Subclass(observer => ...)`.
+export function fixObservableSubclass<
+  S extends new (...args: any[]) => Observable<any>,
+>(subclass: S): S {
+  function set(key: symbol | string) {
+    // Object.defineProperty is necessary because the Symbol.species
+    // property is a getter by default in modern JS environments, so we
+    // can't assign to it with a normal assignment expression.
+    Object.defineProperty(subclass, key, { value: Observable });
+  }
+  if (typeof Symbol === "function" && Symbol.species) {
+    set(Symbol.species);
+  }
+  // The "@@species" string is used as a fake Symbol.species value in some
+  // polyfill systems (including the SymbolSpecies variable used by
+  // zen-observable), so we should set it as well, to be safe.
+  set("@@species");
+  return subclass;
+}