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

Publish Apollo Server packages in ESM #5627

Closed
Tracked by #636
alfaproject opened this issue Aug 19, 2021 · 11 comments
Closed
Tracked by #636

Publish Apollo Server packages in ESM #5627

alfaproject opened this issue Aug 19, 2021 · 11 comments

Comments

@alfaproject
Copy link
Contributor

Currently, the Apollo Server packages are only published as CommonJS but a lot of libraries are starting to switch to ESM.

For people like us that use Serverless functions like AWS Lambda and like to keep our functions lean, we use bundlers to bundle our Serverless function code and dependencies.

We, specifically use Webpack and it could never really tree-shake CommonJS modules properly anyway. There has been some improvements in this regard in Webpack 5 but as far as apollo-server-core is concerned, it never tree shook any of it.

As an example, apollo-server-core reexports some things like graphql-tag or @graphql-tools which would be ok but since these libraries are now publishing in ESM, webpack no longer mixes CommonJS with ESM when bundling so we end up with duplicate packages in our bundles.

Again, that would be ok, except we now have a problem with graphql also being bundled twice due to apollo-server-core being bundled in CommonJS mode which in turn imports the CommonJS version of graphql and then our own use of graphql in ESM mode is also bundled because a library like @graphql-tools now has ESM and imports graphql in ESM mode, so voila we have 2 versions of graphql. This is just an example but you can basically see the whole dependency tree of Apollo Server being in a similar weird state.

To make things worse, we also use @graphql-tools for stitching and therefore import their library as well, so we actually end up with duplicates of @graphql-tools as well because Apollo reexports the CommonJS version. Let me show that with a screenshot of our gateway bundle analysis:
Screenshot+2021-08-19+at+08 21 38

The left side is pretty much all CommonJS modules (or their ESM dependencies) that webpack couldn't quite tree shake safely, and the right side is pretty much all the ESM dependencies that specific lambda is importing and could be concatenated and tree shaken safely. A few points in this screenshot for my plea above:

  1. If you look at the apollo-server-core in the bottom left you will notice that the CommonJS version of @graphql-tools/* was bundled and on the right you will see that the ESM version was also bundled, that's because we also import it and ESM has always priority for better tree-shaking, so this is the first duplicate that bothers me.
  2. Because apollo-server-core is CommonJS only then you will also notice that @graphql-tools/mock is also bundled even though this is a production bundle and Webpack can't get rid of it due to it being CommonJS I assume.
  3. The worst of everything that you see here and that's actually what I spent the last few days debugging is that some parts of graphql are bundled twice, one in CommonJS (.js) and another in ESM (.mjs). If you pay close attention to the graphql bundle on the top right you will see those duplicate files.

None of these points have been bothering me that much because we have been able to keep our gateway smaller than 3mb even with really bad tree shaking, but point number 3 just made us ran into a really weird and difficult to debug edge case that I have been debugging the past few days (ardatan/graphql-tools#3386)

Sorry for the long thread but I hope I can at least convince you guys to consider bundling an ESM version of Apollo Server. I know you guys know how to do it because you are already doing it for Apollo Client and the node ecosystem is moving quite steadily into ESM, so I think this might be the right time. If nothing else, you would make all the Serverless users a happy bunch. <3

@glasser
Copy link
Member

glasser commented Aug 19, 2021

Do you think this is related to #4983 ?

@alfaproject
Copy link
Contributor Author

Don't think so. The last comment on that issue is forcing webpack to bundle the CommonJS version of graphql (.js). What I'm requesting is the exact opposite but without having any of those 'hacks'.

For my exact issue after days of debugging and the help of @ardatan this is actually how I was able to force the ESM version of graphql which could then be tree shaken and not go into conflict with anything else:

  resolve: {
    alias: {
      // Force webpack to resolve the ESM version of `graphql` for better tree
      // shaking and deduplication with `apollo-server-core` CommonJS imports.
      graphql$: require.resolve('graphql/index.mjs'),
    },
  },

If you guys decide to also publish apollo-server-core in ESM, no one that starts using node.js ESM modules will have any issues with any of this anymore if they decide to bundle. (:

@glasser
Copy link
Member

glasser commented Aug 19, 2021

Sure, I'm just asking — do you think the issue you described about multiple graphql copies being bundled (one ESM and one CJS) is the same cause as the issue I linked, not that the workarounds there are the right solution? ie, does fixing this clear up that one too?

I'm definitely in favor of us getting this done. If I understand properly, your proposal is that the published packages will contain both ESM and CJS, so this shouldn't be a backwards-incompatible change at all, right?

While it's true that the Apollo Client team has done something similar and we can crib from them, I'll note that this might get done faster if it's contributed as a PR — the core team definitely has a lot of other things on our plate right now.

@alfaproject
Copy link
Contributor Author

Yes, if Apollo Server had an ESM version bundled I don't think the linked issue would happen because Webpack would have just picked the ESM version of Apollo Server and also the graphql one and then there wouldn't be the need to force the CommonJS version to deduplicate. In that regard, I believe you are right, they are related.

Yes, I was suggesting publishing both the CommonJS version exactly as is today and an extra ESM version as well. So, the .js compilation side by side with the .mjs one.

I'd be happy to do a PR but I'm not comfortable with the Apollo Server code base or building tools, so I'd have to learn that first and then give it a shot but right now I'm a bit busy with work and already got some delays while trying to debug my other issue. ):

For now, I'm glad that you agree with having an ESM bundle out of the box. (:

@ardatan
Copy link

ardatan commented Aug 19, 2021

@alfaproject @glasser You can take a look at our setup in GraphQL Tools and you can even try our bundler Bob. We'd love to help you with that if that sounds good for you :)

@thdxr
Copy link

thdxr commented Jan 18, 2022

Are there any updates on this?

We're trying to include apollo-server-core in our ESM package but it has dynamic requires that break ESM. Not sure why these are needed

@glasser
Copy link
Member

glasser commented Jun 15, 2022

I think I have this working at #6580

@glasser
Copy link
Member

glasser commented Jun 21, 2022

This will be fixed in Apollo Server 4; currently implemented on the version-4 branch.

@glasser glasser closed this as completed Jun 21, 2022
@adikari
Copy link
Contributor

adikari commented Oct 26, 2022

Upgrading to Apollo v4 fixed these issues for me. However, I am running into the same issue when I create the ApolloServer with ApolloGateway. Subgraphs where I am just using ApolloServer, are working fine. I am suspecting the issue is with the gateway package. Can you provide some insights on this @glasser?

@glasser
Copy link
Member

glasser commented Oct 27, 2022

@adikari Hi, I don't believe that @apollo/gateway has made similar changes to Apollo Server to publish that package with ESM-native builds. I'd suggest filing an issue about it on https://github.com/apollographql/federation, making it very clear exactly what issues you run into trying to use Gateway with ESM.

@adikari
Copy link
Contributor

adikari commented Nov 1, 2022

Opened issue in Apollo federation repo apollographql/federation#2226

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants