support inline source map #2795
Conversation
|
Please, give example, and write a test for this use case. |
| report._getSourceMap = (coverage: Profiler.ScriptCoverage) => { | ||
| const path = _url.pathToFileURL(coverage.url.split('?')[0]).href | ||
| const data = sourceMapMeta[path] | ||
|
|
||
| if (!data) | ||
| return {} | ||
| return originalGetSourceMap.call(report, coverage) |
There was a problem hiding this comment.
Is this needed? If the source maps are not found from ctx.vitenode.fetchCache, how would they be present on V8 coverage reports? All files present in V8 reports should have been loaded through vite-node.
In #2786 the source-map-cache will be disabled completely. C8's _getSourceMap won't find source maps in reports then.
There was a problem hiding this comment.
The server side framework will load our code in the runtime. And the entry point is the server side framework, not our code. Therefore it is handled as external.
And since ts-node is used as a require hook, our code actually is processed by ts-node, to-node will generate inline sourcemap and c8 can pick it up.
There was a problem hiding this comment.
Yes, it's similar. Egg.js uses require to load our code.
There was a problem hiding this comment.
With #2786 this will break. I'm not sure where the source maps could be loaded from.
There was a problem hiding this comment.
I'll do some testing with the reproduction case. Maybe with #2786 this would require
--enable-source-mapsor process.setSourceMapsEnabled(true) called manually.
I don't think --enable-source-maps works inside VM, although since we don't process require it shouldn't matter because they are loaded by Node.js itself(?)
As far as I can tell, vitest doesn't use a require hook
Yes, Vitest doesn't intercept require calls and delegates them to Node via createRequire. There are no plans to support require, Vitest's goal is to be ESM first, and supporting require will introduce a lot of complexity. There are better tools to use if you rely on CJS.
There was a problem hiding this comment.
Well, the fix in this PR is to enable vitest to support just in time transpiler like ts-node/tsx. It has been supported by source-map-support and c8 that vitest depends on, therefore the implementation is quite straightforward.
This feature doesn’t require vitest to intercept require to work.
There was a problem hiding this comment.
Supporting this will imply that Vitest can be used with require, which is not true. Some features (like mocking) don't work and are not meant to work (we don't want to receive issues that cannot be resolved). I specifically left out hookRequire, because we don't support it and we don't need additional overhead. If we will ever support require, it should be done in a single action (PR).
I am fine with the coverage change though.
There was a problem hiding this comment.
I am also not against some kind of flag to enable it manually, so users can understand the consequences.
There was a problem hiding this comment.
Now that #2786 is merged, there will be no source-map-cache for c8 to use.
https://github.com/bcoe/c8/blob/2a3d0c7347c77165962de8deca214b077c9166e2/lib/report.js#L183-L184
This PR's change has no effect. The source maps generated by ts-node need to fetched from somewhere.
|
OK, I’ll extract a small example for this case. I’m using Vitest in a big Node project to replace Jest |
|
@sheremet-va @AriPerkkio Please see https://github.com/oliverzy/eggjs-vitest for minimal example. I have put the patch in the repo for quick verification. I'm using ts-node because it's the most popular and reliable tool for ts runtime transpiler |
|
This PR is obosolete since 0.29.0 has changed the source map & v8 coverage implementation a lot. |
Some server side framework like egg.js relies on ts-node to handle TS files.
When using vitest to test such projects, I found the source map is completely missing, because the ts file is not loaded by vitest.
This PR fixes source map issue for such projects that rely on ts-node runtime transpiler.