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.

Sign up for GitHub

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

support inline source map #2795

Closed
wants to merge 1 commit into from
Closed

support inline source map #2795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/coverage-c8/src/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,13 +135,13 @@ export class C8CoverageProvider implements CoverageProvider {
// that we add in packages/vite-node/src/client.ts:114 (vm.runInThisContext)
// TODO: Include our transformations in sourcemaps
const offset = 185

const originalGetSourceMap = report._getSourceMap;
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)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds similar as Fastify which was discussed here: #2028.

I'll do some testing with the reproduction case. Maybe with #2786 this would require --enable-source-maps or process.setSourceMapsEnabled(true) called manually.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it's similar. Egg.js uses require to load our code.

https://github.com/eggjs/egg-core/blob/5cc402347cde8c672a1a448eac7df7b670464df0/lib/utils/index.js#L27

Copy link
Member

Choose a reason for hiding this comment

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

With #2786 this will break. I'm not sure where the source maps could be loaded from.

Copy link
Member

@sheremet-va sheremet-va Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'll do some testing with the reproduction case. Maybe with #2786 this would require --enable-source-maps or 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I am also not against some kind of flag to enable it manually, so users can understand the consequences.

Copy link
Member

Choose a reason for hiding this comment

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

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.


return {
sourceMap: {
Expand Down
1 change: 1 addition & 0 deletions packages/vite-node/src/source-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ export function installSourcemapsSupport(options: InstallSourceMapSupportOptions
install({
environment: 'node',
handleUncaughtExceptions: false,
hookRequire: true,
retrieveSourceMap(source) {
const map = options.getSourceMap(source)
if (map) {
Expand Down