Inline usage reporting protobuf into main package #6515
Merged
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 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 b15bc2c:
|
glasser
force-pushed
the
glasser/inline-usage-reporting-protobuf
branch
from
da39ff4
to
3a5e303
Compare
Keeping it separate mostly just means we have to worry about version skew. It doesn't let us avoid the protobufjs dependency (it was just transitive). Other packages that may need these types (like `@apollo/gateway`) will have a peer dependency on `@apollo/server` anyway. Note that this package consists of .js and .d.ts files that are not generated from .ts files, so they exist outside of `src`. We ensure they end up in the published package via a change to `.npmignore`. As before, the only code that can import the protobuf code should be tests, `import type`s, or code under `src/plugin` that is only evaluated if the plugin is actually used. Fixes #6510.
glasser
force-pushed
the
glasser/inline-usage-reporting-protobuf
branch
from
3a5e303
to
b15bc2c
Compare
trevor-scheer
approved these changes
Jun 3, 2022
glasser
added a commit
that referenced
this pull request
Jun 10, 2022
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.
glasser
added a commit
that referenced
this pull request
Jun 13, 2022
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.
glasser
added a commit
that referenced
this pull request
Jun 15, 2022
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.
glasser
added a commit
that referenced
this pull request
Jun 15, 2022
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.
glasser
added a commit
that referenced
this pull request
Jun 16, 2022
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.
glasser
added a commit
that referenced
this pull request
Jun 16, 2022
* Revert "Inline usage reporting protobuf into main package (#6515)" 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. * plugins: Use dynamic import instead of manual intermediate functions 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! * Generate ESM and CJS files; change to deep-import API 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). * scripts to subdir * Make @apollo/usage-reporting-protobuf dual CJS/ESM too 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Keeping it separate mostly just means we have to worry about version
skew. It doesn't let us avoid the protobufjs dependency (it was just
transitive). Other packages that may need these types (like
@apollo/gateway) will have a peer dependency on@apollo/serveranyway.
Note that this package consists of .js and .d.ts files that are not
generated from .ts files, so they exist outside of
src. We ensure theyend up in the published package via a change to
.npmignore.As before, the only code that can import the protobuf code should be
tests,
import types, or code undersrc/pluginthat is only evaluatedif the plugin is actually used.
Also fixes missing dependencies on cors and body-parser needed for
the standalone server. (As part of #6243 we may make sure that these are
only needed if you actually import the standalone server directory.)
Fixes #6510.