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

chore(deps): upgrade @types/pino to be compatible with latest sonic-stream types #592

Merged
merged 1 commit into from Jul 28, 2021

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • upgrade @types/pino to 6.3.11

@legendecas legendecas requested a review from a team as a code owner July 28, 2021 06:55
@legendecas legendecas linked an issue Jul 28, 2021 that may be closed by this pull request
@Flarna
Copy link
Member

Flarna commented Jul 28, 2021

Does this still work for users using an older version of @types/pino?

A while ago we had a discussion if we should pin the version here or not. Or even use * to accept any version selected in the final app.

@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #592 (b3c97d4) into main (68e5449) will not change coverage.
The diff coverage is n/a.

❗ Current head b3c97d4 differs from pull request most recent head 80a20b8. Consider uploading reports for the commit 80a20b8 to get more accurate results

@@           Coverage Diff           @@
##             main     #592   +/-   ##
=======================================
  Coverage   94.78%   94.78%           
=======================================
  Files         179      179           
  Lines       10976    10976           
  Branches     1088     1088           
=======================================
  Hits        10404    10404           
  Misses        572      572           

@legendecas
Copy link
Member Author

legendecas commented Jul 28, 2021

@Flarna Does this still work for users using an older version of @types/pino?

IIUC, types maintained in DefinitelyTyped versioned with the same minor.patch following the original package (see https://github.com/DefinitelyTyped/DefinitelyTyped#how-do-definitely-typed-package-versions-relate-to-versions-of-the-corresponding-library). If there are no mistakes, the version of type packages should follow semantic versioning.

I'm not sure which condition you are referring to. If you mean users that declared their own @types/pino dependencies in their package, as long as the user-defined version range is compatible with the version defined in @opentelemetry/instrumentation-pino, it should be fine.

@legendecas
Copy link
Member Author

Or even use * to accept any version selected in the final app.

IIUC, the instrumentation package should set a version range of pino that it supports (like say, declare a set of version range in peer dependencies). A * sounds not great to me.

@Flarna
Copy link
Member

Flarna commented Jul 28, 2021

@types packages follow major.minor of their original. They may add/remove/rework definitions during this resulting in breakage like here. So in short @types packages don't follow semver (and this is documented at DefinitelyTyped).

I'm not sure which condition you are referring to. If you mean users that declared their own @types/pino dependencies in their package, as long as the user-defined version range is compatible with the version defined in @opentelemetry/instrumentation-pino, it should be fine.

I mean the version selected by the end user. @opentelemetry/instrumentation-pino uses a fixed version therefore a mismatch is likely. In that case NPM installs both versions (one as child of @opentelemetry/instrumentation-pino and one in the root node_modules folder).

@dyladan
Copy link
Member

dyladan commented Jul 28, 2021

Merging this since it is fixing the build which breaks in every PR

@dyladan dyladan merged commit fc70b9a into open-telemetry:main Jul 28, 2021
@legendecas legendecas deleted the fix-instrument-pino branch July 28, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@opentelemetry/instrumentation-pino failed to compile
3 participants