Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat!: set compilation target to
es2022
for all packages but api and semantic-conventions #5456feat!: set compilation target to
es2022
for all packages but api and semantic-conventions #5456Changes from 14 commits
539c406
1d785c1
8111dec
204d0a0
6d83f15
515416d
3679793
8f57ec2
778a9ea
9adc09a
81276fb
0d0e6cc
f0aec32
231bd5a
fd7581e
27a07a7
f2ec659
b2fdca2
e80e5ae
8c608c7
56ae3ee
5cd8397
795d676
08af24d
db71061
bd55845
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any test failure on "main". I'm not sure what the issue was here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, something seems to be wrong here. 🤔
For some reason, the Instrumentation's metrics instruments seem to be
undefined
without these changes, which should never be the case. Seems like a bug that surfaced from changing the compile settings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this._updateMetricInstruments()
is called but does not define the properties on theHttpInstrumentation
instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
once the base constructor returns, the properties are all
undefined
. 🤔There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is due to https://www.typescriptlang.org/tsconfig/#useDefineForClassFields and a simplest fix could be adding the
declare
modifier to derived instrumentation properties:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this diff:
and the instr-http tests pass again.
The diff in the generated JS is:
Pretty subtle :)
The best full write-up of what is going on is: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#the-usedefineforclassfields-flag-and-the-declare-property-modifier
(linked to from @legendecas' link above). Thanks for the pointer.
commit db71061
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my feeble memory, the issue is basically this: