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 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": { 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/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, + }); + }); }); 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 b0d04939f77..0126650b051 100644 --- a/src/utilities/observables/Concast.ts +++ b/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 | PromiseLike; @@ -55,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); @@ -66,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), @@ -143,7 +151,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; @@ -238,16 +246,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 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); 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 new file mode 100644 index 00000000000..d0c7f4a71dd --- /dev/null +++ b/src/utilities/observables/__tests__/subclassing.ts @@ -0,0 +1,45 @@ +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([ + 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"]); + }); + }); +}); 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; +}