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

Use zen-observable as ESM #5961

Closed
abdonrd opened this issue Feb 18, 2020 · 11 comments
Closed

Use zen-observable as ESM #5961

abdonrd opened this issue Feb 18, 2020 · 11 comments

Comments

@abdonrd
Copy link
Contributor

abdonrd commented Feb 18, 2020

The last version of zen-observable has an ESM export: esm.js.

You can also check it at unpkg:
https://unpkg.com/browse/zen-observable@0.8.15/

It would be great that Apollo Client uses that instead of the CJS export (index.js).

That way, zen-observable works directly in the browser.

@benjamn
Copy link
Member

benjamn commented Feb 18, 2020

Here's where we import Observable from "zen-observable". As you can see, we're not doing anything that would force your bundler to import the CommonJS entry point, and the version constraint in our package.json (^0.8.14) permits installing the latest version of zen-observable, which is just 0.8.15. In short, I don't think Apollo Client is standing in the way of using the new ESM entry point. Let me know if there's something concrete you want us to do here.

@billyjanitsch
Copy link

The problem is that zen-observable doesn't actually point to its ESM build in its package.json so a bundler can't know that it has one. That package would need to add "module": "esm.js" to its package.json for this to work.

I wonder if this is also the cause of #5686, although I don't know what would have changed between apollo-boost and @apollo/client to cause this in one but not the other.

@benjamn
Copy link
Member

benjamn commented Feb 19, 2020

ESM support is a very recent addition to zen-observable, so I'm sure they would appreciate an issue/PR to add the "module": "esm.js" entry point to package.json. You can certainly link back to this issue as supporting evidence, but I believe it's their responsibility to follow the same conventions that other CJS/ESM packages follow.

@bennypowers
Copy link
Contributor

There's a use case of apollo-client without a bundler that's worth considering here. Buildless workflows and even smaller applications that use unbundled CDN and http2.

I believe post-3.0 ac is moving away from observable, but in the mean time it would be nice to have an all-browser option to cover those use cases.

@abdonrd
Copy link
Contributor Author

abdonrd commented Jul 20, 2020

@benjamn since the maintainer has not responded for a long time... Would you be interested in changing the package if I publish it under a scope? Something like @abdonrd/zen-observable. Just with this PR to use an ESM build.

benjamn added a commit that referenced this issue 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:
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 added a commit that referenced this issue 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:
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
Copy link
Member

benjamn commented Jan 25, 2021

@abdonrd Thanks for following through and opening that PR! Since there's been no response, I'm moving ahead with a wrapping strategy, using the package name zen-observable-ts: #7615. If your PR ever gets merged (🤞 ), we can go back to using zen-observable directly.

These changes should be released in Apollo Client v3.4 (the next minor version).

@benjamn benjamn added this to the Release 3.4 milestone Jan 25, 2021
@PowerKiKi
Copy link
Contributor

What about finally moving to rxjs, which is a very capable, well maintained and tree shake library ?

It is so good that it is already used in Apollo client tests code (!). Moving to rxjs would unify your codebase while reducing bundle size of Angular projects, most likely not increasing React project size. And last but not least it would reduce your maintenance work.

I'm more than willing to create a pr for that, if you give me the go.

@PowerKiKi
Copy link
Contributor

PowerKiKi commented Jan 25, 2021

I guess the answer to my question is #5749

PS: btw, thanks benjamn for your very informative commit message. I know we don't always feel like taking extra time for that, but yours are an invaluable source of information 🎉

@abdonrd
Copy link
Contributor Author

abdonrd commented Feb 9, 2021

Closed by #7615, right?

Just fast-json-stable-stringify as ESM to have all deps as ESM! 🤞🏻

@benjamn
Copy link
Member

benjamn commented Feb 17, 2021

@abdonrd Yes! However, please note that #7615 is only in the v3.4 betas at this point.

Edit: In addition to fast-json-stable-stringify (which I have a plan to replace), there's this little package called react that does not export ESM modules, though you can avoid that by importing from @apollo/client/core.

@benjamn benjamn closed this as completed Feb 17, 2021
@abdonrd
Copy link
Contributor Author

abdonrd commented Feb 17, 2021

@benjamn yes, looking forward to the release!

I'm very glad to read that there will be a solution for fast-json-stable-stringify! 👏🏻

"this little package called react". Yeah, little package 😂
Personally I think the best option would be to remove React from @apollo/client (that works for any) and have something like @apollo/client/react with React. But yes, for now I use @apollo/client/core.

benjamn added a commit that referenced this issue May 14, 2021
> Note: I am expecting tests to fail for this commit, demonstrating the
importance of using a stable serialization strategy for field arguments.

Although fast-json-stable-stringify has done its job well, it only
provides a "main" field in its package.json file, pointing to a CommonJS
entry point, and does not appear to export any ECMAScript modules.

Thanks to our conversion/abandonment of fast-json-stable-stringify and
other CommonJS-only npm dependencies (zen-observable in #5961 and
graphql-tag in #6074), it should now (after this PR is merged) be
possible to load @apollo/client/core from an ESM-aware CDN like
jsdelivr.net or jspm.io:

  <script type=module
    src="https://cdn.jsdelivr.net/npm/@apollo/client@beta/core/+esm">
  </script>

If you put that script tag in an HTML file, or inject it into the DOM of
any webpage, you will currently see this error:

  Uncaught SyntaxError: The requested module
  '/npm/fast-json-stable-stringify@2.1.0/+esm' does not provide an
  export named 'default'

This list of errors used to be longer, but now the only package left is
fast-json-stable-stringify.

Note that we're loading @apollo/client/core@beta here, not
@apollo/client@beta. The reason @apollo/client itself is not yet
ESM-ready is that react and react-dom are the two remaining
CommonJS-only dependencies, and @apollo/client currently/regrettably
re-exports utilities from @apollo/client/react.

If importing from @apollo/client/core is a burden or feels weird to you,
please know that we are planning to make @apollo/client synonymous with
@apollo/client/core in Apollo Client 4.0, along with making
@apollo/client/react synonymous with the v3 API of @apollo/client,
though of course those will be major breaking changes:
#8190
@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 a pull request may close this issue.

6 participants