Build dual ESM/CJS packages; move some exports to deep imports #6580
Conversation
✅ Deploy Preview for apollo-server-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 64b2d63:
|
glasser
force-pushed
the
glasser/esm-experiment
branch
3 times, most recently
from
2933fb4
to
2d5a61b
Compare
packages/server/src/ApolloServer.ts
Outdated
| const { | ||
| ApolloServerPluginLandingPageLocalDefault, | ||
| ApolloServerPluginLandingPageProductionDefault, | ||
| } = await import('./plugin/landingPage/default'); | ||
| const plugin: ApolloServerPlugin<TContext> = isDev | ||
| ? ApolloServerPluginLandingPageLocalDefault() | ||
| : ApolloServerPluginLandingPageProductionDefault(); |
There was a problem hiding this comment.
Is there any benefit to importing these individually inline instead of both of them? My guess is no, but I'm going to expand this question a bit for my own understanding.
- The majority of any overhead from this is almost certainly in the dynamic import of the file, so grabbing 1 or n things from the same file makes a negligible difference?
isDevisn't known at build time so there's no opportunity for dead code elimination by splitting up the imports?
There was a problem hiding this comment.
I don't think there's any difference since they are both defined in the same small file (and share a lot of code).
| @@ -57,6 +50,17 @@ import type { | |||
| } from '.'; | |||
| import { mockLogger } from '../mockLogger'; | |||
| import gql from 'graphql-tag'; | |||
| import { | |||
| ApolloServerPluginUsageReportingOptions, | |||
There was a problem hiding this comment.
Huh, our test files aren't required to import type? Or is that only required when the import also pertains to runtime? I hadn't noticed this before if so.
There was a problem hiding this comment.
We use ApolloServerPluginUsageReporting as a value.
Looks like the ability to make specific imports type within a line is newer than import type as a whole, and is mostly designed for the combination of two particular compiler options that we don't use: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html
Um I thought I hadn't thought about this before but apparently not: microsoft/TypeScript#45998 (comment)
eslint doesn't give you a way to mandate adding type on individual imports that I can find (https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/consistent-type-imports.md) and I don't know that VSCode makes it easy either. Actually I bet eslint can't do it because it requires looking at more than one file at a time... there's typescript-eslint/typescript-eslint#4338 but this is just about not having multiple import lines, not ensuring there are types when needed.
| @@ -1,4 +1,4 @@ | |||
| import { ApolloServer } from '../..'; | |||
There was a problem hiding this comment.
This is weird, right? Was this just wrong before?
There was a problem hiding this comment.
I think it probably worked before because of, I dunno, something in the ../.. package.json that made it find the right thing?
packages/server/src/index.ts
Outdated
| @@ -8,12 +8,5 @@ export { | |||
|
|
|||
| export { ApolloServer } from './ApolloServer'; | |||
| export { expressMiddleware } from './express'; | |||
There was a problem hiding this comment.
Should this also be its own entry point?
There was a problem hiding this comment.
On the one hand, this has no runtime dependencies, so breaking it out isn't a big deal for dependency taming.
On the other hand, Express v5 might actually happen someday! expressjs/express#4920 https://github.com/expressjs/express/blob/5.x/History.md Which might actually make backwards incompatible changes? idk? So maybe we should export it from @apollo/server/express4, or at least from @apollo/server/express with @apollo/server/express5 available later? Thoughts?
package.json
Outdated
| "precompile": "node precompile.mjs", | ||
| "postcompile": "node postcompile.mjs", |
There was a problem hiding this comment.
NBD: Can we put these in a scripts folder?
| @@ -1,12 +1,15 @@ | |||
| export { ApolloServer } from './ApolloServer.js'; | |||
| // Note that this is purely a type export. | |||
| export * from './externalTypes/index.js'; | |||
There was a problem hiding this comment.
But still needs the .js extension? Maybe there's not a way to export type * from ... and the .js isn't hurting anything?
There was a problem hiding this comment.
Yeah, no export type *: microsoft/TypeScript#48508
| import { Trace, google } from '@apollo/usage-reporting-protobuf'; | ||
| import proto from '@apollo/usage-reporting-protobuf'; |
There was a problem hiding this comment.
Why doesn't the previous way of doing this work? (Assuming it doesn't since I've seen this change twice now and I'm curious)
There was a problem hiding this comment.
When you import CJS from ESM, Node has special code that analyzes the CJS file and tries to figure out what the exports are. This package of ours was too complex for it to analyze, between the additional index.js file that just imports the generated code and disables a Long feature of protobufjs, and the exact way that exports are set up in generated code itself.
I made some tweaks to https://github.com/apollographql/protobuf.js to help with this. First, Long support is now just off by default, so the wrapper file can go away. Second, turns out that protobuf.js sorta supports ESM generation, though it needed a few small improvements. So now @apollo/usage-reporting-protobuf is itself a dual ESM/CJS package, and the old normal imports work.
| private nodes = new Map<string, Trace.Node>([ | ||
| private nodes = new Map<string, proto.Trace.Node>([ |
There was a problem hiding this comment.
Is this change needed? You destructured Trace up above.
There was a problem hiding this comment.
Same in other places below.
| @@ -1,14 +1,14 @@ | |||
| import type { WithRequired } from '@apollo/utils.withrequired'; | |||
| import { json } from 'body-parser'; | |||
| import bodyParser from 'body-parser'; // note that importing 'json' directly doesn't work in ESM | |||
There was a problem hiding this comment.
Ah, is this the same thing happening with proto?
There was a problem hiding this comment.
Essentially, but since we don't own body-parser it's harder to fix :)
This one was the motivation for me to make the smoke test, because it was a runtime failure.
| @@ -0,0 +1,39 @@ | |||
| const { ApolloServer } = require('@apollo/server'); | |||
There was a problem hiding this comment.
Just a thought, would it be possible / beneficial if we ran these built packages through the integration test suite? Not sure how that looks bringing them into jest land. Overall this is pretty great that we're testing the actual builds!
There was a problem hiding this comment.
Not sure how to do that but it would be cool!
This reverts commit 0b58585. This leaves the fix of adding cors and body-parser to server's deps and leaves the removal of a TODO(AS4) about this. The build process for this package is completely different from a normal TS package, so keeping it together seems best. This seems like it'll make setting up ESM builds easier.
We want to avoid loading usage reporting protobuf until as late as possible. Previously we had a specific file src/plugins/index.ts full of little wrapper functions that call `require`. Now we use dynamic `import` (which is compiled to CJS as `require` by tsc) instead. We use "exports" in package.json to let you do deep imports from particular files (only). Note that this still only supports CJS!
The overall approach is to ask tsc to build two different projects for package/server, one with CJS output and one with ESM, outputting into two subdirectories of packages/server/dist. package.json teaches Node how to find various entry points with both ESM and CJS. To make the ESM version work, this equires making all "file imports" (not `import type` or imports of packages) end with `.js`. Instead of using `__dirname` we now update the version number in a normal TS file before building. We have to do some weird stuff with how we import the proto package since it is not ESM-y enough due to the weird index.js in it. (Maybe we should just fix our protobuf port to drop long support directly so we can have a nicer index.js which is analyzed properly?) We update the actual API so that `startStandaloneServer` must be imported from `@apollo/server/standalone` (and thus express can be tree-shaken if you don't use the standalone server) and so that the various plugins are imported from `@apollo/server/plugin/usageReporting` etc (and so you don't have to load protobuf stuff if you don't use it).
This mostly involved running pbjs a second time to generate ESM. A few tweaks were made to our fork `@apollo/protobufjs`: - Long support is just turned off by default, so we don't need the wrapper index.* files that just turn it off - The import line in generated code no longer includes `* as` since that didn't quite work in practice (it imported an object with a 'default' key on it) - The `@apollo/protobufjs` package.json uses `exports` to show where `/minimal` is
glasser
force-pushed
the
glasser/esm-experiment
branch
from
05b3b26
to
64b2d63
Compare
There were a few problems with #6580, which added ESM builds and deep imports to version-4. The `exports` section of `packages/server/package.json` had two mistakes: the `types` lines linked to files in the cjs directory but we only placed them in the esm directory, and the `types` lines are apparently supposed to appear first in each block. However, it turned out that this didn't matter because tsc doesn't even look in the `exports` section unless you set `moduleResolution` to `node16` or `nodenext` in your tsconfig `compilerOptions`, and we weren't! And it turns out doing that setting is a bit of a pain because many packages don't currently work with that flag. So if we published our package with the deep imports only listed in `exports` in package.json, we'd break any TypeScript user who's on the normal `moduleResolution: "node"` and telling them to set `nodenext` would be hard to obey. So since we want to continue to support tsc with `moduleResolution: "node"`, we have to teach tsc about our deep imports using the strategy of "spew a bunch of package.jsons around the package", not even under `src`/`dist`. Fortunately these package.json files can use paths starting with `../` in order to find the actual generated files. So for example, `standalone/package.json` tells tsc how to find `../dist/standalone/index.d.ts`, via fields like `types` and `main`. You might think that now that we have these little package.json files, the `exports` block would be completely redundant. Not so! Node itself does not support deep imports like `@apollo/server/standalone` by looking for a `main` in `standalone/package.json`: when importing from a published package name, it only looks for `main` at the top level of the package. So to support deep imports in Node we need the `exports` block! So this PR leaves both the `exports` block and the tree of `package.json`s in place, to be consumed by different software. We add some tsc use cases to the new smoke test. Note that it is not a goal of this PR to ensure that `moduleResolution: "nodenext"` works.
There were a few problems with #6580, which added ESM builds and deep imports to version-4. The `exports` section of `packages/server/package.json` had two mistakes: the `types` lines linked to files in the cjs directory but we only placed them in the esm directory, and the `types` lines are apparently supposed to appear first in each block. However, it turned out that this didn't matter because tsc doesn't even look in the `exports` section unless you set `moduleResolution` to `node16` or `nodenext` in your tsconfig `compilerOptions`, and we weren't! And it turns out doing that setting is a bit of a pain because many packages don't currently work with that flag. So if we published our package with the deep imports only listed in `exports` in package.json, we'd break any TypeScript user who's on the normal `moduleResolution: "node"` and telling them to set `nodenext` would be hard to obey. So since we want to continue to support tsc with `moduleResolution: "node"`, we have to teach tsc about our deep imports using the strategy of "spew a bunch of package.jsons around the package", not even under `src`/`dist`. Fortunately these package.json files can use paths starting with `../` in order to find the actual generated files. So for example, `standalone/package.json` tells tsc how to find `../dist/standalone/index.d.ts`, via fields like `types` and `main`. You might think that now that we have these little package.json files, the `exports` block would be completely redundant. Not so! Node itself does not support deep imports like `@apollo/server/standalone` by looking for a `main` in `standalone/package.json`: when importing from a published package name, it only looks for `main` at the top level of the package. So to support deep imports in Node we need the `exports` block! So this PR leaves both the `exports` block and the tree of `package.json`s in place, to be consumed by different software. We add some tsc use cases to the new smoke test. Our smoke tests verify that consumers with `moduleResolution: "nodenext"` work (or at least, that they will work once ardatan/graphql-tools#4532 is released). However, our own builds don't work that way next because some of our test-only dependencies don't work with that flag.
See individual commits.
Fixes #6243. Fixes #5627.