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(instrumentation-graphql): remove graphql/types dependency #754

Merged
merged 12 commits into from
Dec 14, 2021

Conversation

nata7che
Copy link
Contributor

@nata7che nata7che commented Nov 23, 2021

Which problem is this PR solving?

Fixes Graphql is added as main dependency by instrumentation and may case version collision

Short description of the changes

This pr removes the @graphql/types dependency from the package.json and modifies references.

@nata7che nata7che requested a review from a team November 23, 2021 12:14
@github-actions github-actions bot requested a review from obecny November 23, 2021 12:15
@codecov
Copy link

codecov bot commented Nov 23, 2021

Codecov Report

Merging #754 (e273546) into main (27e37e3) will decrease coverage by 0.68%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #754      +/-   ##
==========================================
- Coverage   94.90%   94.21%   -0.69%     
==========================================
  Files          13       24      +11     
  Lines         707     1695     +988     
  Branches      142      236      +94     
==========================================
+ Hits          671     1597     +926     
- Misses         36       98      +62     
Impacted Files Coverage Δ
...opentelemetry-instrumentation-graphql/src/types.ts 100.00% <ø> (ø)
...opentelemetry-instrumentation-graphql/src/utils.ts 93.40% <ø> (ø)
...try-instrumentation-graphql/src/instrumentation.ts 91.61% <100.00%> (ø)
.../opentelemetry-instrumentation-graphql/src/enum.ts 100.00% <0.00%> (ø)
...entelemetry-instrumentation-graphql/src/symbols.ts 100.00% <0.00%> (ø)
...entelemetry-instrumentation-graphql/test/helper.ts 100.00% <0.00%> (ø)
...entelemetry-instrumentation-graphql/test/schema.ts 55.84% <0.00%> (ø)
...ages/auto-instrumentations-node/test/utils.test.ts 96.87% <0.00%> (ø)
...metry-instrumentation-graphql/test/graphql.test.ts 100.00% <0.00%> (ø)
... and 4 more

@Flarna
Copy link
Member

Flarna commented Nov 23, 2021

This is a revert of #468 for graphql.
@obecny please take a look. As far as I remember you had quite a strong opinion regarding this topic.

@nata7che
Copy link
Contributor Author

Not really sure why codecov/project is failing:
image

@nata7che
Copy link
Contributor Author

This is a revert of #468 for graphql. @obecny please take a look. As far as I remember you had quite a strong opinion regarding this topic.

@Flarna this is actually not a revert but a fix (if I understand correctly) since the @graphql/types is deprecated and not needed anymore and as @graphql provides the types.
image

@Flarna
Copy link
Member

Flarna commented Nov 30, 2021

The point in #468 was that types must be a dependency (not dev-dependency) otherwise users may run in typescript issues.

As @types/graphql is deprecated (because graphql itself now contains also the types) it would mean that graphql should be a dependency.
Actually it was already one before as @types/graphql has graphql as dependency.

Therefore it is more or less a revert as it moves types from dependency back into dev-dependency.

Personally l'm fine with the change as I think the instrumenation should not have such a dependendy as the risk is high to end up with two versions of graphql. But the decission made by the maintainers in #468 was a different one.

@nata7che
Copy link
Contributor Author

nata7che commented Nov 30, 2021

@Flarna I agree, this is actually what happens now (2 versions of graphql) and what this PR aims to fix 👍🏻
Thanks for your time!

@dyladan
Copy link
Member

dyladan commented Nov 30, 2021

I don't see graphql in the dependencies at all now

@nata7che
Copy link
Contributor Author

nata7che commented Dec 1, 2021

The bottom line is that currently, the instrumentation (not-dev) requires its instrumented library. IMHO this is a big no-no that should be avoided at all costs. This is in fact causing issues to customers and hence the opened bug.

@nata7che nata7che changed the title fix: remove graphql/types dependency fix (instrumentation-graphql): remove graphql/types dependency Dec 1, 2021
@nata7che nata7che changed the title fix (instrumentation-graphql): remove graphql/types dependency fix(instrumentation-graphql): remove graphql/types dependency Dec 1, 2021
@nata7che
Copy link
Contributor Author

nata7che commented Dec 5, 2021

Hi, @obecny, would highly appreciate your input on this 🙏🏻 we'd love to push this forward, as it creates a break in some of our usages due to versions conflict.

@obecny
Copy link
Member

obecny commented Dec 5, 2021

there was a bug with certain ts version and because of this bug the meta packages were unable to be used if you don't include certain type even if you don't use it. So if this bug is still valid then removing types will simply cause the bug to happen again. If this is causing another bug then it should be simply vary how to prevent one and another bug and come out with some solution for both situations. Not sure what's the best way of fixing then 2 bugs at the same time, but whatever will be decided the both bug should be taken into consideration. After this change I would encourage to check the meta packages if the are still working fine

@nata7che
Copy link
Contributor Author

nata7che commented Dec 6, 2021

Thanks, @obecny! do you happen to remember which bug was this and point us to it?
In the meanwhile, how should we proceed regarding this PR? Do you think we can merge it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graphql is added as dependency by instrumentation and may case version collision
4 participants