fix: do not instrument the top level module#225
Conversation
This makes sure ts-node works as expected, since it needs to verify if the top level is the main module. That also applies to other CLI tools. Fixes: nodejs#174 Fixes: nodejs#34
9221346 to
aaed0a1
Compare
I seem to remember that Open Telemetry AWS serverless hooks an absolute path to the entry module |
|
Here is where the "entry" filename gets used for hooking aws-lambda: I'm unsure if this file is actually considered the entry module though! |
|
I had a look at it and that filename is not the entrypoint, if I am not completely mistaken. |
timfish
left a comment
There was a problem hiding this comment.
Yep, good point. This shouldn't ever be required.
| - name: Verify Minimum Coverage Is Met | ||
| run: > | ||
| lcov --summary all_lcov.info | grep lines | cut -d' ' -f 4 | cut -d% -f 1 | xargs node -e "x=process.argv[1];console.log(x);assert(+x >= 90)" | ||
| lcov --summary all_lcov.info | grep lines | cut -d' ' -f 4 | cut -d% -f 1 | xargs node -e "x=process.argv[1];console.log(x);assert(+x >= 80)" |
There was a problem hiding this comment.
This is a significant drop in accepted coverage. Are we unable to cover the things in this PR?
There was a problem hiding this comment.
I am uncertain why the job failed the second time, I did not check the coverage locally. I believe the third argument changes are not reflected in the coverage. I would bump that afterwards again
|
Interestingly, I broke our Datadog hook tests by including this. We (Datadog) do something incorrect in our hook when using the The question for me is just if we should revert it on v2, release v3 and include it there, since the change needs adjustments on our side. @timfish @jsumners-nr optinions? |
Sounds like a good plan! |
This makes sure ts-node works as expected, since it needs to verify if the top level is the main module. That also applies to other CLI tools.
This could be considered a breaking change, while I can not think of a good reason why anyone would ever want to instrument a top level file.
Fixes: #174
Fixes: #34
This works for Node.js 16+, while I am unsure how to fix this in older versions.