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

[AS-223] Use forked version of protobufjs #3530

Merged
merged 7 commits into from
Nov 20, 2019

Conversation

Enrico2
Copy link
Contributor

@Enrico2 Enrico2 commented Nov 19, 2019

Use a the forked version of protobufjs in Apollo Server (https://github.com/apollographql/protobuf.js)

We want to do this for a few reasons:

  1. first and foremost, a user is suffering from a bug where we override a global property in protobufjs (long override), and they want the default behavior in another package.

  2. We want to introduce some functionality to this package soon. (David's PR: Allow plain JS object repeated fields to use toArray() method protobufjs/protobuf.js#1302)

Copy link
Member

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks for putting this together.

Just leaving some artifacts here in this PR to create references to the commit/line which stepped on Long (d8ade4d#diff-d5d9fcc27f8062d80afd9d8a291b85e6R7) and the PR that introduced it (#2900) for posterity's sake!

We'll want to add a CHANGELOG.md entry before we merge this. I'll suggest this relatively straight-forward and bland wording, but I think it basically encapsulates the change:

- `apollo-engine-reporting`: Swap usage of `protobufjs` for a newly published fork located
  at [`@apollo/protobufjs`](https://npm.im/@apollo/protobufjs).  This is to account for
  the [relative uncertainty](https://github.com/protobufjs/protobuf.js/issues/1199)
  into the continued on-going maintenance of the official `protobuf.js` project. 
  This should immediately resolve a bug that affected `Long` types in
  `apollo-engine-reporting` and other non-Apollo projects that rely on `protobuf.js`'s
  `Long` type.  [PR #3530](https://github.com/apollographql/apollo-server/pull/3530)

Copy link
Member

@trevor-scheer trevor-scheer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@trevor-scheer trevor-scheer merged commit 37503d2 into master Nov 20, 2019
@trevor-scheer trevor-scheer deleted the ran/use_forked_protobuf branch November 20, 2019 21:59
@abernix abernix added this to the Release 2.9.12 milestone Dec 17, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 16, 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

Successfully merging this pull request may close these issues.

3 participants