feat: added support for got v12, v13, v14 #1157
Conversation
aryamohanan
force-pushed
the
got-esm
branch
2 times, most recently
from
1645756
to
c4e622b
Compare
Lets stick to our naming convention.
See convention or even better this. |
| @@ -35,8 +35,8 @@ app.get('/', (req, res) => { | |||
|
|
|||
| app.get('/multipleRequireWithStealthyRequire', async (req, res) => { | |||
| // Wrap the require calls with stealthyRequire to avoid caching | |||
| const firstInstanceOfGot = stealthyRequire(require.cache, () => require('got')); | |||
There was a problem hiding this comment.
Suggestion: How about we choose a different library for this test case?
| * })(); | ||
| * NOTE: From v12 the got module is pure ESM. | ||
| * See https://github.com/sindresorhus/got/releases/tag/v12.0.0. | ||
| * In ESM test, we use the latest got module. |
There was a problem hiding this comment.
issue: I am missing a detailed explanation in the requireHook.js why got >= v12 is working.
aryamohanan
changed the title
aryamohanan
force-pushed
the
got-esm
branch
2 times, most recently
from
f51495b
to
d80b866
Compare
aryamohanan
changed the title
| // We aim to extract the module name to apply our instrumentation. | ||
| // The native ESM support is pending, and the requireHook isn't compatible with native ESM. | ||
| // However, for the 'got' module, we persist with the require hook as its uses the core HTTP | ||
| // module under the hood. In our instrumentation, 'got' is automatically included alongside |
There was a problem hiding this comment.
issue: I think the description is not accurate and hard to follow.
with the require hook as its uses the core HTTP module under the hood
got is using http2wrapper, which is a CJS library, which requires http. Only thats why our requireHook works.
If got would only use import http from 'node:http' it would not work.
Its kinda a coincidence that got >= v12 is working.
There was a problem hiding this comment.
I updated the comment little bit, could you check again please?
| // CASE: we ignore all file endings, which we are not interested in. Any module can load any file. | ||
| // CASE: The native ESM support is pending, and the requireHook isn't compatible with native ESM. | ||
| // However, 'got' is automatically instrumented along with the the http2wrapper, |
There was a problem hiding this comment.
| // However, 'got' is automatically instrumented along with the the http2wrapper, | |
| // However, 'got' is instrumented by coincidency, because it is requiring the [http2wrapper](https://github.com/search?q=repo%3Asindresorhus%2Fgot%20from%20%27http2-wrapper%27&type=code) CJS package, which is requiring http. |
| // CASE: we ignore all file endings, which we are not interested in. Any module can load any file. | ||
| // CASE: The native ESM support is pending, and the requireHook isn't compatible with native ESM. | ||
| // However, 'got' is automatically instrumented along with the the http2wrapper, | ||
| // which is a CommonJS library requiring http. Despite 'got' transitioning to a pure ESM module |
There was a problem hiding this comment.
| // which is a CommonJS library requiring http. Despite 'got' transitioning to a pure ESM module | |
| // Despite 'got' transitioning to a pure ESM module |
| // However, 'got' is instrumented by coincidency, because it is requiring the http2wrapper | ||
| // https://github.com/search?q=repo%3Asindresorhus%2Fgot%20from%20%27http2-wrapper%27&type=code | ||
| // CJS package, which is requiring http. Despite 'got' transitioning to a pure ESM module | ||
| // from v12 onwards, it continues to work with the existing requireHook. |
There was a problem hiding this comment.
I just noticed that the explanation is not the underlying reason:
because it is requiring the http2wrapper
got and node-fetch work because we are instrumenting http and https without even using the requireHook. That is different to every other instrumentation we have. We even require http and https on top. Could you please rewrite the comment? Please also add the link.
But We still should keep the explanation that ESM libraries do not trigger requireHook when importing core modules and as soon as an ESM library imports a CJS package, our requireHook will get triggered.
There was a problem hiding this comment.
I've up[dated the comment
Added Native ESM Support for got Module
In our instrumentation setup, while native ESM support is still pending and the requireHook isn't compatible with native ESM, the 'got' module transitions to a pure ESM module from version 12 onwards. This transition remains functional due to 'got' being instrumented alongside the 'http' module, which works without the requireHook. Notably, this differs from our other instrumentations, which require 'http' and 'https' at the top. Importantly, native ESM libraries importing core Node modules do not trigger Module._load and thus do not utilize the requireHook. However, our requireHook is triggered when an ESM library imports a CommonJS package.
We've extended our testing suite to cover this latest version, ensuring accurate capture of spans.