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(plugin-http): typings #545

Conversation

OlivierAlbertini
Copy link
Member

closes open-telemetry#541

Signed-off-by: Olivier Albertini <[email protected]>
Signed-off-by: Olivier Albertini <[email protected]>
@codecov-io
Copy link

codecov-io commented Nov 16, 2019

Codecov Report

Merging #545 into master will decrease coverage by 2.75%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #545      +/-   ##
==========================================
- Coverage   92.98%   90.22%   -2.76%     
==========================================
  Files         147      144       -3     
  Lines        7694     7162     -532     
  Branches      654      649       -5     
==========================================
- Hits         7154     6462     -692     
- Misses        540      700     +160
Impacted Files Coverage Δ
...opentelemetry-plugin-http/test/utils/assertSpan.ts 100% <ø> (ø) ⬆️
...lemetry-plugin-http/test/functionals/utils.test.ts 99.34% <ø> (ø) ⬆️
...-plugin-http/test/integrations/http-enable.test.ts 92.92% <ø> (ø) ⬆️
packages/opentelemetry-plugin-http/src/utils.ts 100% <100%> (ø) ⬆️
...pentelemetry-plugin-http/test/utils/httpRequest.ts 90.9% <100%> (ø) ⬆️
packages/opentelemetry-plugin-http/src/http.ts 97.35% <100%> (+0.02%) ⬆️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
... and 39 more

@Flarna
Copy link
Member

Flarna commented Nov 17, 2019

You could wait until DefinitelyTyped/DefinitelyTyped#40430 is merged and a new version of @types/node got published as this would avoid several casts.

I have already a version but not created a PR yet as it would no pass CI at this time.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, one question, when this this will be merged, will it fail again?, or this will need to be modified and applied ?

@dyladan
Copy link
Member

dyladan commented Nov 18, 2019

I would prefer @Flarna's suggestion to wait for DefinitelyTyped/DefinitelyTyped#40430 to merge as it would simplify this PR and require fewer casts. It is a popular package that has passed all checks and has multiple approvals so I expect it will be published very soon.

@mayurkale22
Copy link
Member

mayurkale22 commented Nov 18, 2019

I would prefer @Flarna's suggestion to wait for DefinitelyTyped/DefinitelyTyped#40430 to merge as it would simplify this PR and require fewer casts. It is a popular package that has passed all checks and has multiple approvals so I expect it will be published very soon.

Although I agree with your comment, I am not sure when will they cut a new release. This will also block other PRs, either we can remove the HTTP plugin from the build process or merge this PR and revisit once DefinitelyTyped/DefinitelyTyped#40430 is released or use lower version of @types/node. WDYT?

Edit 1: I tried with @types/node => 12.7.9, looks like a compilation step is working well.

@Flarna
Copy link
Member

Flarna commented Nov 18, 2019

A new version of types packages is published automatically after a PR is merged. Hopefully that happens during the next hours but no gurantee.

In my projects I tend to use a fixed version of @types/node and updated it every now and then manually. It happens quite easy that something get's broken otherwise - just like this time via some minor null/undefined tuning.

@Flarna
Copy link
Member

Flarna commented Nov 18, 2019

@obecny Even with the new version of @types/node changes are needed in the http/https plugins but fewer changes. I haven't tested the proposed version here against the future @types/node version.

@Flarna
Copy link
Member

Flarna commented Nov 18, 2019

12.12.9 has been published, I created an alternative PR, see #548

@OlivierAlbertini
Copy link
Member Author

I agree

I would prefer @Flarna's suggestion to wait for DefinitelyTyped/DefinitelyTyped#40430 to merge as it would simplify this PR and require fewer casts. It is a popular package that has passed all checks and has multiple approvals so I expect it will be published very soon.

Although I agree with your comment, I am not sure when will they cut a new release. This will also block other PRs, either we can remove the HTTP plugin from the build process or merge this PR and revisit once DefinitelyTyped/DefinitelyTyped#40430 is released or use lower version of @types/node. WDYT?

Edit 1: I tried with @types/node => 12.7.9, looks like a compilation step is working well.

Indeed, it would be another solution! I make this PR in the meantime of DefinitelyTyped/DefinitelyTyped#40430 but it was quick!

@OlivierAlbertini OlivierAlbertini deleted the feature/fix-node-type branch November 18, 2019 22:23
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.

http/https plugins are not compatible to @types/node 12.12.8
6 participants