From ab6b60a6f404c3df34aa657bb5643d72bf9fc59a Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Tue, 1 Dec 2020 20:13:49 -0500 Subject: [PATCH 1/7] Set both Symbol.species and "@@species" on Concast constructor (#7403) An attempt to address the problems I speculated about in https://github.com/apollographql/apollo-client/issues/6520#issuecomment-736899757 Theory: in the Hermes JS engine, Symbol.species is not defined, so Object.defineProperty was not called, but perhaps "@@species" was getting set somewhere else by a polyfill library, causing the zen-observable library to fall back to "@@species" instead of using/ignoring the nonexistent Symbol.species symbol. --- src/utilities/observables/Concast.ts | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/src/utilities/observables/Concast.ts b/src/utilities/observables/Concast.ts index b0d04939f77..2949a699d5e 100644 --- a/src/utilities/observables/Concast.ts +++ b/src/utilities/observables/Concast.ts @@ -243,11 +243,19 @@ export class Concast extends Observable { // 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 => ...)`. +// Symbol.species property 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 => ...)`. +function setSpecies(key: symbol | string) { + // Object.defineProperty is necessary because Concast[Symbol.species] + // is a getter by default in modern JS environments, so we can't + // assign to it with a normal assignment expression. + Object.defineProperty(Concast, key, { value: Observable }); +} if (typeof Symbol === "function" && Symbol.species) { - Object.defineProperty(Concast, Symbol.species, { - value: Observable, - }); + setSpecies(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. +setSpecies("@@species"); From 6d7a8eb4e5cd0cdf96c592c1f59c97378965226e Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 11:09:32 -0500 Subject: [PATCH 2/7] Fix minor type error for private Concast#resolve function. --- src/utilities/observables/Concast.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/utilities/observables/Concast.ts b/src/utilities/observables/Concast.ts index 2949a699d5e..111707ae0bb 100644 --- a/src/utilities/observables/Concast.ts +++ b/src/utilities/observables/Concast.ts @@ -143,7 +143,7 @@ export class Concast extends Observable { // 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) => void; private reject: (reason: any) => void; public readonly promise = new Promise((resolve, reject) => { this.resolve = resolve; From 78d9074053aa6b385de154c36c8e125721edf924 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 10:47:06 -0500 Subject: [PATCH 3/7] Extract fixObservableSubclass helper function. --- src/__tests__/__snapshots__/exports.ts.snap | 1 + src/utilities/index.ts | 1 + src/utilities/observables/Concast.ts | 25 +++------------ .../observables/__tests__/subclassing.ts | 31 +++++++++++++++++++ src/utilities/observables/subclassing.ts | 28 +++++++++++++++++ 5 files changed, 65 insertions(+), 21 deletions(-) create mode 100644 src/utilities/observables/__tests__/subclassing.ts create mode 100644 src/utilities/observables/subclassing.ts diff --git a/src/__tests__/__snapshots__/exports.ts.snap b/src/__tests__/__snapshots__/exports.ts.snap index 1830f4a440b..5f90346e03f 100644 --- a/src/__tests__/__snapshots__/exports.ts.snap +++ b/src/__tests__/__snapshots__/exports.ts.snap @@ -326,6 +326,7 @@ Array [ "compact", "concatPagination", "createFragmentMap", + "fixObservableSubclass", "getDefaultValues", "getDirectiveNames", "getFragmentDefinition", diff --git a/src/utilities/index.ts b/src/utilities/index.ts index 8f4cf5b1696..7617a9187b3 100644 --- a/src/utilities/index.ts +++ b/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'; diff --git a/src/utilities/observables/Concast.ts b/src/utilities/observables/Concast.ts index 111707ae0bb..3cf2d520bd2 100644 --- a/src/utilities/observables/Concast.ts +++ b/src/utilities/observables/Concast.ts @@ -1,5 +1,6 @@ import { Observable, Observer, ObservableSubscription } from "./Observable"; import { iterateObserversSafely } from "./iteration"; +import { fixObservableSubclass } from "./subclassing"; type MaybeAsync = T | PromiseLike; @@ -238,24 +239,6 @@ export class Concast extends Observable { } } -// 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 property 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 => ...)`. -function setSpecies(key: symbol | string) { - // Object.defineProperty is necessary because Concast[Symbol.species] - // is a getter by default in modern JS environments, so we can't - // assign to it with a normal assignment expression. - Object.defineProperty(Concast, key, { value: Observable }); -} -if (typeof Symbol === "function" && Symbol.species) { - setSpecies(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. -setSpecies("@@species"); +// Necessary because the Concast constructor has a different signature +// than the Observable constructor. +fixObservableSubclass(Concast); diff --git a/src/utilities/observables/__tests__/subclassing.ts b/src/utilities/observables/__tests__/subclassing.ts new file mode 100644 index 00000000000..f6560a65af5 --- /dev/null +++ b/src/utilities/observables/__tests__/subclassing.ts @@ -0,0 +1,31 @@ +import { Observable } from "../Observable"; +import { Concast } from "../Concast"; + +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 new Promise((resolve, reject) => { + const doubles: number[] = []; + mapped.subscribe({ + next(n) { + doubles.push(n); + }, + error: reject, + complete() { + resolve(doubles); + } + }); + }).then(doubles => { + expect(doubles).toEqual([2, 4, 6, 8, 10]); + }); + }); +}); diff --git a/src/utilities/observables/subclassing.ts b/src/utilities/observables/subclassing.ts new file mode 100644 index 00000000000..d4d65c1ad47 --- /dev/null +++ b/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, +>(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; +} From f1f924845ffd9822fcccbb0de01c1b303a4923f8 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 11:29:47 -0500 Subject: [PATCH 4/7] Allow Concast constructor to be invoked with a Subscriber. --- src/utilities/observables/Concast.ts | 11 +++++- src/utilities/observables/Observable.ts | 1 + .../observables/__tests__/subclassing.ts | 38 +++++++++++++------ 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/utilities/observables/Concast.ts b/src/utilities/observables/Concast.ts index 3cf2d520bd2..0126650b051 100644 --- a/src/utilities/observables/Concast.ts +++ b/src/utilities/observables/Concast.ts @@ -1,4 +1,4 @@ -import { Observable, Observer, ObservableSubscription } from "./Observable"; +import { Observable, Observer, ObservableSubscription, Subscriber } from "./Observable"; import { iterateObserversSafely } from "./iteration"; import { fixObservableSubclass } from "./subclassing"; @@ -56,7 +56,7 @@ export class Concast extends Observable { // 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>) { + constructor(sources: MaybeAsync> | Subscriber) { super(observer => { this.addObserver(observer); return () => this.removeObserver(observer); @@ -67,6 +67,13 @@ export class Concast extends Observable { // 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), diff --git a/src/utilities/observables/Observable.ts b/src/utilities/observables/Observable.ts index fd721adcedd..7a83c20ed34 100644 --- a/src/utilities/observables/Observable.ts +++ b/src/utilities/observables/Observable.ts @@ -6,6 +6,7 @@ import 'symbol-observable'; export type ObservableSubscription = ZenObservable.Subscription; export type Observer = ZenObservable.Observer; +export type Subscriber = ZenObservable.Subscriber; // Use global module augmentation to add RxJS interop functionality. By // using this approach (instead of subclassing `Observable` and adding an diff --git a/src/utilities/observables/__tests__/subclassing.ts b/src/utilities/observables/__tests__/subclassing.ts index f6560a65af5..d0c7f4a71dd 100644 --- a/src/utilities/observables/__tests__/subclassing.ts +++ b/src/utilities/observables/__tests__/subclassing.ts @@ -1,6 +1,21 @@ import { Observable } from "../Observable"; import { Concast } from "../Concast"; +function toArrayPromise(observable: Observable): Promise { + return new Promise((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([ @@ -13,19 +28,18 @@ describe("Observable subclassing", () => { expect(mapped).toBeInstanceOf(Observable); expect(mapped).not.toBeInstanceOf(Concast); - return new Promise((resolve, reject) => { - const doubles: number[] = []; - mapped.subscribe({ - next(n) { - doubles.push(n); - }, - error: reject, - complete() { - resolve(doubles); - } - }); - }).then(doubles => { + 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"]); + }); + }); }); From 1b89257e133f0c86689157b5546084d67f201218 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 10:49:29 -0500 Subject: [PATCH 5/7] Use fixObservableSubclass for ObservableQuery, too. --- src/core/ObservableQuery.ts | 5 ++++ src/core/__tests__/ObservableQuery.ts | 41 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 6378ffd425a..2d2d8f09249 100644 --- a/src/core/ObservableQuery.ts +++ b/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); } diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index c220cb36895..6656b1fffc4 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/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, + }); + }); }); From 7648cf6f383d20e4213946a60b95db880177440b Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 11:55:17 -0500 Subject: [PATCH 6/7] Increase bundlesize limit from 25.5kB to 25.6kB. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index e296cad3d45..f2d26385b88 100644 --- a/package.json +++ b/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": { From fc57af8bfdc662cae96cf6f37809a312df4349b7 Mon Sep 17 00:00:00 2001 From: Ben Newman Date: Fri, 5 Feb 2021 12:10:09 -0500 Subject: [PATCH 7/7] Mention PRs #7403 and #7660 in CHANGELOG.md. --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b664d0851c4..32aba5e14af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ - Reactivate forgotten reactive variables whenever `InMemoryCache` acquires its first watcher.
[@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).
+ [@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