-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support subclassing Observable with non-class constructor functions. #7640
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Now that the zen-observable-ts package has the ability to export Observable as a native class (#7615), we need to be careful when extending Observable using classes (like ObservableQuery and Concast) that have been compiled to ES5 constructor functions (rather than native classes), because the generated _super.call(this, subscriber) code throws when _super is a native class constructor (#7635). Rather than attempting to change the way the TypeScript compiler transforms super(subscriber) calls, this commit wraps Observable.call and Observable.apply to work as expected, by using Reflect.construct to invoke the superclass constructor correctly, when the Reflect API is available: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/construct Another option would be to ship native class syntax with @apollo/client, by changing the "target" in tsconfig.json from "es5" to "es2015" or later, so that consumers of @apollo/client would be forced to compile native class syntax however they see fit. That would be a more disruptive change, in part because it would prevent subclassing Apollo Client-defined classes using anything other than native class syntax and/or the Reflect.construct API, which is the very same problem this commit is trying to fix for the Observable class.
Another option: I could move this logic into |
benjamn
added a commit
to apollographql/zen-observable-ts
that referenced
this pull request
Feb 2, 2021
jcreighton
approved these changes
Feb 2, 2021
benjamn
added a commit
to apollographql/zen-observable-ts
that referenced
this pull request
Feb 2, 2021
benjamn
added a commit
to apollographql/zen-observable-ts
that referenced
this pull request
Feb 2, 2021
benjamn
added a commit
to apollographql/zen-observable-ts
that referenced
this pull request
Feb 2, 2021
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Now that the
zen-observable-ts
package has the ability to exportObservable
as a native ECMAScriptclass
(#7615), we need to be careful when extendingObservable
using classes (likeObservableQuery
andConcast
) that have been compiled to ES5 constructor functions (rather than native classes), because the generated_super.call(this, subscriber)
code throws when_super
is a native class constructor (#7635).Rather than attempting to change the way the TypeScript compiler transforms
super(subscriber)
calls, this commit wrapsObservable.call
andObservable.apply
to work as expected, usingReflect.construct
to invoke the superclass constructor correctly, when theReflect
API is available.Another option would be to ship native
class
andsuper
syntax with@apollo/client
, by changing the "target" intsconfig.json
from "es5" to "es2015" or later, so consumers of@apollo/client
code would be forced to compile nativeclass
syntax however they see fit. That would be a more disruptive change, in part because it would prevent subclassing Apollo Client-exported classes using anything other than nativeclass
syntax and/or theReflect.construct
API, which is the very same problem this commit is trying to fix for theObservable
class.