Internalize zen-observable types and drop @types/zen-observable dependency
#152
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
This allows us to remove the `@types/zen-observable` package entirely, which should help with apollographql/apollo-client#8688.
I don't think we need this, as long as TypeScript doesn't complain about our use of `Symbol.observable` in the `.d.ts` files. Apollo Client imports a `symbol-observable` polyfill here: https://github.com/apollographql/apollo-client/blob/main/src/utilities/observables/Observable.ts
We implemented these methods explicitly in PR #1, but then PR #105 went back to compiling `Observable` down to ES5 constructor functions. In other words, the current implementations of `Observable` (both CommonJS and ESM) automatically have these static methods, and they work just like `Function.prototype.{call,apply}`, because `Observable` is compiled in both cases from a `class` to a constructor function.
This was referenced Aug 24, 2021
|
Makes sense LGTM. |
benjamn
added a commit
that referenced
this pull request
Aug 24, 2021
Minor version bump due to potential refactoring risk from PR #152. This version will be published as `next` until we're confident we can promote it to `latest`.
benjamn
added a commit
to apollographql/apollo-client
that referenced
this pull request
Aug 24, 2021
benjamn
added a commit
to apollographql/apollo-client
that referenced
this pull request
Aug 24, 2021
…pes/zen-observable` (#8695) Includes apollographql/zen-observable-ts#152 Should fix #8688
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is another approach to solving apollographql/apollo-client#8688 entirely within the
zen-observable-tspackage, which does not involve first converting the codebase to TypeScript as #151 attempted (so far unsuccessfully) to do.As I wrote in apollographql/apollo-client#8688 (comment), I believe this is a problem
zen-observable-tscan and should solve within itself, without@apollo/clienthaving to declare a dependency on@types/zen-observablein order to use types fromzen-observable-ts.Unfortunately, as long as
zen-observable-tsuses types from@types/zen-observable, it appears some (yarn v2-using) consumers ofzen-observable-tswill be forced to install@types/zen-observableas a direct dependency of their own. This includes the@apollo/clientpackage itself, which depends directly onzen-observable-tsand uses/exposes theObservabletype it exports in many places.To solve this problem, this PR removes
@types/zen-observablefrom the picture, and internalizes the.d.tsfiles that it provided inmodule.d.ts, without global declarations like theZenObservablenamespace orSymbol.observable.Note that
zen-observableitself is not a TypeScript project, so@types/zen-observableis merely a description of its existing JavaScript API, rather than a generated artifact of thezen-observablebuild process. In that sense, internalizing those types withinzen-observable-tsseems safe and legitimate, since these types are just another description of whatzen-observableprovides.Note that @brainkim and I previously internalized the implementation of
zen-observable/esm.jsin PR #105, so now the types are following suit.