Skip to content
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

Revive zen-observable-ts wrapper for zen-observable package. #7615

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jan 25, 2021

Apollo Client currently uses the Observable implementation provided by the zen-observable npm package, since it is small and works well for our needs. Although zen-observable itself is not written in TypeScript, we use the companion package @types/zen-observable to provide types.

We are actively considering switching from zen-observable to RxJS (#5749), though it remains to be seen how much of a breaking change that will be (and thus whether it makes sense for a minor or major version update).

In the meantime, several issues (most recently #5961) have been opened pointing out that zen-observable effectively does not support importing its code as ECMAScript modules, even though it has a separate entry point (zen-observable/esm.js) that uses ESM syntax.

This is a problem the zen-observable package could easily fix by adding a "module" field to its package.json, but @abdonrd tried to propose that change in a PR (as I requested), and has not heard back since June 2020.

Fortunately, Apollo maintains an npm package called zen-observable-ts, which was originally created to provide TypeScript types, before @types/zen-observable was introduced. This commit revives that wrapper package, so we can make sure both CommonJS and ESM exports are supported, with full TypeScript types, until the zen-observable maintainer gets around to merging that PR.

I considered forking zen-observable, but I'm happy with this wrapping strategy, since reusing zen-observable makes it easier to take advantage of any future updates to the zen-observable package, and also preserves the appropriate npm download credit (whatever that may be worth).

I also considered using patch-package to apply changes to node_modules/zen-observable/package.json upon installation, but that doesn't really work unless @apollo/client bundles the zen-observable implementation into itself, since otherwise the original (unpatched) zen-observable package will continue to be installed when developers npm install @apollo/client. The patch-package tool is great, but I don't think it's meant for libraries to use.

Thankfully, this time around we do not need to hand-write the TypeScript types, since they can be re-exported from @types/zen-observable.

I bumped the major version of zen-observable-ts, since the older versions (used by apollo-link) still get more than two million downloads per week. The source code for the zen-observable-ts package can now be found at https://github.com/apollographql/zen-observable-ts, rather than the old https://github.com/apollographql/apollo-link monorepo.

Apollo Client currently uses the Observable implementation provided by the
zen-observable npm package, since it is small and works well for our
needs. Although zen-observable itself is not written in TypeScript, we
use the companion package @types/zen-observable to provide types.

We are actively considering switching from zen-observable to RxJS (#5749),
though it remains to be seen how much of a breaking change that will be
(and thus whether it makes sense for a minor or major version update).

In the meantime, several issues (most recently #5961) have been opened
pointing out that zen-observable effectively does not support importing
its code as ECMAScript modules, even though it has a separate entry point
(zen-observable/esm.js) that uses ESM syntax.

This is a problem the zen-observable package could easily fix by adding a
"module" field to its package.json, but @abdonrd tried to propose that
change in a PR (as I requested), and has not heard back since June 2020:
zenparsing/zen-observable#74

Fortunately, Apollo maintains an npm package called zen-observable-ts,
which was originally created to provide TypeScript types, before
@types/zen-observable was introduced. This commit revives that wrapper
package, so we can make sure both CommonJS and ESM exports are supported,
with full TypeScript types, until the zen-observable maintainer gets
around to merging that PR.

I considered forking zen-observable, but I'm happy with this wrapping
strategy, since reusing zen-observable makes it easier to take advantage
of any future updates to the zen-observable package.

I also considered using https://www.npmjs.com/package/patch-package to
apply changes to node_modules/zen-observable/package.json upon
installation, but that doesn't really work unless @apollo/client bundles
the zen-observable implementation into itself, since otherwise the
original (unpatched) zen-observable package will continue to be installed
when developers npm install @apollo/client. The patch-package tool is
great, but I don't think it's meant for libraries to use.

Thankfully, this time around we do not need to hand-write the TypeScript
types, since they can be re-exported from @types/zen-observable.

I bumped the major version of zen-observable-ts, since the older versions
(used by apollo-link) still get more than two million downloads per week.

The source code for the zen-observable-ts package can now be found at
https://github.com/apollographql/zen-observable-ts, rather than the old
https://github.com/apollographql/apollo-link monorepo.
@benjamn benjamn force-pushed the esm-zen-observable branch from 05fe908 to 848fca2 Compare January 25, 2021 21:39
@benjamn benjamn changed the title Revive zen-observable-ts wrapper. Revive zen-observable-ts wrapper for zen-observable package. Jan 26, 2021
@hwillson
Copy link
Member

Hmmm - @benjamn the file size check was failing due to bundlesize getting a 404 when trying to talk to Github. I ran a few quick tests after SSH'ing into circle, and for some reason there seems to be a problem with the BUNDLESIZE_GITHUB_TOKEN we're using. Removing that token let the tests complete properly, which is very odd. I put the BUNDLESIZE_GITHUB_TOKEN environment variable back, but just an FYI in case you notice this weirdness again.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff @benjamn - thanks!

@benjamn
Copy link
Member Author

benjamn commented Jan 26, 2021

@hwillson Thanks for digging into the bundlesize failure—that was baffling me.

@benjamn benjamn merged commit 1ad9042 into release-3.4 Jan 26, 2021
@benjamn benjamn deleted the esm-zen-observable branch January 26, 2021 15:55
@benjamn
Copy link
Member Author

benjamn commented Jan 27, 2021

You can tell this is working by importing from the URL https://cdn.jsdelivr.net/npm/zen-observable-ts@1.0.0-beta.4/+esm in a web browser. Neither https://cdn.jsdelivr.net/npm/zen-observable-ts/+esm nor https://cdn.jsdelivr.net/npm/zen-observable/+esm load successfully.

benjamn added a commit that referenced this pull request Feb 2, 2021
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.
@hwillson hwillson removed this from the MM-2021-06 milestone Jul 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants