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

fix(apollo-reporting-protobuf): added long to dependencies #4841

Closed
wants to merge 1 commit into from
Closed

fix(apollo-reporting-protobuf): added long to dependencies #4841

wants to merge 1 commit into from

Conversation

kiancross
Copy link

The code emitted by the protobufjs pbts tool includes the long dependency. This should be a peer dependency of protobufjs (done in apollographql/protobuf.js#6) and therefore also needs to be added to the dependencies of apollo-reporting-protobuf.

@kiancross kiancross changed the title fix(apollo-reporting-protobuf) added long to dependencies fix(apollo-reporting-protobuf): added long to dependencies Jan 9, 2021
@andreialecu
Copy link

andreialecu commented Jan 28, 2021

I was also looking into this because it breaks on yarn2 in PnP mode.

There's one caveat here though. apollo-reporting-protobuf does not even use the Long types, and it opts out of it via --force-number:

"pbjs": "apollo-pbjs --target static-module --out dist/protobuf.js --wrap commonjs --force-number dist/reports.proto",

However, https://cdn.jsdelivr.net/npm/apollo-reporting-protobuf@0.6.2/dist/protobuf.d.ts imports long on the first line, but doesn't use it anywhere else within (because of forcing number above).

I think the right fix would be to omit adding that import if Long types are opted out of in:

https://github.com/apollographql/protobuf.js/blob/757f6e3a6ccad26bde1192d4eb9da7d0ff0e8b2e/cli/pbts.js#L154-L157

@andreialecu
Copy link

I opened apollographql/protobuf.js#7 which should make the changes in this PR unnecessary.

@kiancross kiancross closed this Jan 28, 2021
@kiancross kiancross deleted the add-long-deps branch January 28, 2021 09:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 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.

2 participants