Skip to content

Commit

Permalink
apollo-reporting-protobuf: check in generated code, regenerate manually
Browse files Browse the repository at this point in the history
Today, the `marked` project pushed a minor release that broke Node 12
compatibility: markedjs/marked#2106

`marked` is a dep of `jsdoc` which is a dep of the protobufjs CLI.
Unfortunately it turns out that the protobufjs CLI installs dependencies
including `jsdoc` *at runtime*, *unversioned*. Yikes! See `setup`
https://github.com/apollographql/protobuf.js/blob/master/cli/util.js

I tried changing our fork to not do this and move the deps to be dev
deps of this project (see `@apollo/[email protected]`) but that didn't
work out on the first try. So instead, let's just not require the
protobufjs CLI to work on Node 12, and also start saving time by not
constantly regenerating the code in this package. Now code is only
generated manually (`lerna run generate`) and checked in, and it can be
done on the latest Node.
  • Loading branch information
glasser committed Jun 16, 2021
1 parent db1c55d commit 65db07d
Show file tree
Hide file tree
Showing 9 changed files with 10,700 additions and 480 deletions.
455 changes: 1 addition & 454 deletions package-lock.json

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions packages/apollo-reporting-protobuf/.npmignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
*
!src/**/*
!dist/**/*
dist/**/*.test.*
!generated/**/*
generated/**/*.test.*
!package.json
!README.md
25 changes: 17 additions & 8 deletions packages/apollo-reporting-protobuf/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,21 @@ Apollo.
> We'd happily accept a PR which makes the appropriate changes!
Currently, this package generates a majority of its code with
[`protobufjs`](https://www.npmjs.com/package/protobufjs) based on the
`reports.proto` file. The output is generated with the `prepare` npm script.
`@apollo/protobufjs` (a fork of
[`protobufjs`](https://www.npmjs.com/package/protobufjs) that we maintain
specifically for this package) based on the `reports.proto` file. The output is
generated with the `generate` npm script.

The root of the repository provides the `devDependencies` necessary to build
these definitions (e.g. `pbjs`, `pbts`, `protobuf`, etc.) and the `prepare`
npm script is invoked programmatically via the monorepo tooling (e.g. Lerna)
thanks to _this_ module's `postinstall` script. Therefore, when making
changes to this module, `npx lerna run prepare` should be run from the **root**
of this monorepo in order to update the definitions in _this_ module.
The root of the repository provides some `devDependencies` necessary to build
these definitions qand the `prepare` npm script is invoked programmatically via
the monorepo tooling (e.g. Lerna) thanks to _this_ module's `postinstall`
script. Therefore, when making changes to this module, run scripts via `npx
lerna run SCRIPTNAME` in the **root** of this monorepo in order to update the
definitions in _this_ module.

To update `reports.proto` to the current version recognized by the Studio usage
reporting ingress, run `lerna run update-proto`. To then regenerate the JS and
TS files, run `npx lerna run generate`. We check in the generated code and only
regenerate it manually, partially to make builds faster (no need to run pbjs on
every `npm install`) and partially so that we don't have to make sure that
`pbjs` runs on every Node version that we support.
2 changes: 2 additions & 0 deletions packages/apollo-reporting-protobuf/generated/index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import * as protobuf from './protobuf';
export = protobuf;
12 changes: 12 additions & 0 deletions packages/apollo-reporting-protobuf/generated/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const protobuf = require('./protobuf');
const protobufJS = require('@apollo/protobufjs/minimal');

// Remove Long support. Our uint64s tend to be small (less
// than 104 days).
// XXX Just remove this in our fork? We already deleted
// the generation of Long in protobuf.d.ts in the fork.
// https://github.com/protobufjs/protobuf.js/issues/1253
protobufJS.util.Long = undefined;
protobufJS.configure();

module.exports = protobuf;
Loading

0 comments on commit 65db07d

Please sign in to comment.