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: bump typescripts to 4.4.4 to fix compile issue with @types/koa #1616

Merged
merged 2 commits into from
Aug 2, 2023

Conversation

chigia001
Copy link
Contributor

@chigia001 chigia001 commented Jul 27, 2023

BEGIN_COMMIT_OVERRIDE
fix: fix typescript compilation issue with koa types
END_COMMIT_OVERRIDE

Which problem is this PR solving?

  • tsc in instrumentation-cucumber and instrumentation-aws-sdk is older than other package. When run tsc -p .for instrumentation-aws-sdk @types/koa is type check but the new version of @types/koa required at least [email protected] to work

Short description of the changes

  • bump typescript for instrumentation-cucumber and instrumentation-aws-sdk to 4.4.4

EDIT: enable explainFiles in tsconfig show this message:

Entry point for implicit type library 'koa' with packageId '@types/koa/[email protected]'

So clearly tsc is loading all @types/* package in node_modules

@codecov
Copy link

codecov bot commented Jul 27, 2023

Codecov Report

Merging #1616 (62067d9) into main (db50d00) will decrease coverage by 1.10%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1616      +/-   ##
==========================================
- Coverage   96.06%   94.96%   -1.10%     
==========================================
  Files          14       18       +4     
  Lines         914     1171     +257     
  Branches      199      237      +38     
==========================================
+ Hits          878     1112     +234     
- Misses         36       59      +23     

see 4 files with indirect coverage changes

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR. I think since instrumentation-aws-sdk is experimental, and instrumentation-cucumber is not yet released, it's fine to bump typescript; we'll need to fix this to get #1611 out the door.

PTAL @Ugzuzg (component owner for cucumber), @blumamir @carolabadeer (component owner for aws-sdk)

Copy link
Member

@blumamir blumamir left a comment

Choose a reason for hiding this comment

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

I always favor aligning the dependencies in the monorepo to use the same version for all packages.

For cucumber, I guess the version is older since the PR was opened long time ago and I think it should be bumped.

For aws-sdk, I am not aware of why it was not bumped in #1374 but that seems to me like it was unintentionally missed and it make sense to me to bump it as well.

One thing I don't yet understand is why tsc complains about koa when the dependency for the koa instrumentation is aligned correctly. Is it related to dependency hoisting?

@blumamir blumamir merged commit a53f643 into open-telemetry:main Aug 2, 2023
@chigia001
Copy link
Contributor Author

chigia001 commented Aug 2, 2023

One thing I don't yet understand is why tsc complains about koa when the dependency for the koa instrumentation is aligned correctly. Is it related to dependency hoisting?

Yes, we hoist on @types/* to root directory and tsc will check all @types/* in both your current package's node_modules folder and root node_modules folder.

@chigia001 chigia001 deleted the pin-typescript-version branch August 13, 2023 05:08
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.

5 participants