fix: use relative paths in `sources` for transformed source maps #12079

bmeurer · 2023-02-16T13:45:35Z

Description

This refines the fix from #4985 to
turn absolute paths into relative paths for the sources array in
source maps for transformer outputs (and thereby completely avoids the
Windows drive letter problem). In order to minimize unintended negative
side effects, we perform this step only when the source file name is
absolute.

This addresses the issue that source files show up with an absolute path
prefix in case of Vue¹.

Additional context

Bug: https://crbug.com/1411596
Ref: #4964
Ref: #4912

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

cc @jecfish

https://goo.gle/devtools-vite-interoperability ↩

benmccann · 2023-02-16T13:58:51Z

Let's make sure to run this PR against the ecosystem CI once the Vite tests are passing

patak-dev · 2023-02-16T20:25:47Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-02-16T20:26:10Z

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	✅ success
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
previewjs	✅ success
qwik	❌ failure
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	✅ success
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	❌ failure
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

patak-dev · 2023-02-17T08:30:49Z

About ecosystem-ci: the Qwik fail should be unrelated, and the vite-plugin-vue fail is due to outdated snapshots.

sapphi-red

I tested this PR with plugin-vue and plugin-react.

By running playground/vue in plugin-vue repo with this branch, I saw some of them working and some of them not working. (playground/vue doesn't have css.devSourcemap enabled)

The result with playground/react in plugin-react was:

I guess these requires a fix on the plugin side and I think it's fine to merge this PR.
Though I think we should not merge this until the minor beta period as this requires other plugins to update their sourcemap handlings.

bmeurer · 2023-02-17T13:11:07Z

How do I "run" the playgrounds?

sapphi-red · 2023-02-17T16:41:14Z

How do I "run" the playgrounds?

The steps to run the playground of plugin-vue/plugin-react repos are:

Open Vite repo
Switch to this branch
pnpm i && pnpm build
Clone plugin-vue/plugin-react repo
Add pnpm.overrides.vite: '../vite/packages/vite' to package.json
pnpm i && pnpm build in plugin-vue/plugin-react repo
pnpm dev in playground/vue

bmeurer · 2023-02-19T10:59:27Z

The @fs, components, hmr and context prefixes, are these artifacts of the playground setup? @bluwy mentioned that at least @fs is prepended due to the file: dependency in the package.json.

sapphi-red · 2023-02-19T14:13:37Z

No, I don't think these are artifacts of the playground setup. I guess sources field of the sourcemap of components/Dummy.jsx has C:/Users/green/foobar and Chrome resolves that as components/C:/Users/green/foobar.

bmeurer · 2023-02-20T08:36:03Z

No, I don't think these are artifacts of the playground setup. I guess sources field of the sourcemap of components/Dummy.jsx has C:/Users/green/foobar and Chrome resolves that as components/C:/Users/green/foobar.

Ooohhhh, you're right. It never occurred to me that we also need to pay special attention to potential Windows paths in sourcemaps. I'll dig into that (https://crbug.com/1417564).

bmeurer · 2023-02-20T09:22:44Z

No, I don't think these are artifacts of the playground setup. I guess sources field of the sourcemap of components/Dummy.jsx has C:/Users/green/foobar and Chrome resolves that as components/C:/Users/green/foobar.

Ooohhhh, you're right. It never occurred to me that we also need to pay special attention to potential Windows paths in sourcemaps. I'll dig into that (https://crbug.com/1417564).

Quick follow up here: After some discussion with the team, there's nothing we can do about this in DevTools, since c: is a valid URL path component (and a valid Unix folder name), so resolving it relative as done here is the correct behavior for DevTools.

The correct fix here is to avoid putting absolute (Windows) paths into source maps in the first place.

Ref: vitejs/vite#12079

bmeurer · 2023-02-21T12:58:35Z

I have a fix for the vite-plugin-vue in here. It basically does the same as this PR, cleaning up the sources after the compiler ran.

bmeurer · 2023-02-23T17:24:33Z

I was just discussing this with @danielroe and we might need to move this a layer up. Please don't merge this yet.

bmeurer · 2023-02-23T21:46:59Z

Based on the session earlier with @danielroe, I now changed the patch to hook into the choke-point after the transformer is called, which seems to completely fix it for Vue and React!

patak-dev · 2023-02-24T12:08:24Z

packages/vite/src/node/server/transformRequest.ts

+        map.sources[sourcesIndex] = path.relative(
+          path.dirname(mod.file),
+          sourcePath,
+        )


@sapphi-red just checking here that this path.relative call won't be affected by issues like the one you were worried about with paths in different drives on windows that can't be expressed as relative paths. I assume the path would be returned as absolute in this case. To be honest, maybe we should document having Vite work across hard drives as out of scope if this will increase complexity for everyone else.

Windows also has this concept of relative paths with drive letters, which I think would also break vite.

I suppose instead of using path.relative here, we could also do string gymnastics ourselves, but that likely would be even worse / unpredictable.

I assume the path would be returned as absolute in this case.

Yes, you're correct. I think we can ignore the cross drive usage as it doesn't work in many places already at least for now.

This refines the fix from vitejs#4985 to turn absolute paths into relative paths for the `sources` array in source maps for transformer outputs (and thereby completely avoids the Windows drive letter problem). In order to minimize unintended negative side effects, we perform this step only when the source file name is absolute. This addresses the issue that source files show up with an absolute path prefix in case of Vue[^1]. Bug: https://crbug.com/1411596 Ref: vitejs#4964 Ref: vitejs#4912 [^1]: https://goo.gle/devtools-vite-interoperability

patak-dev · 2023-02-24T14:41:35Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-02-24T14:42:02Z

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
histoire	❌ failure
iles	✅ success
ladle	✅ success
laravel	✅ success
marko	✅ success
nuxt-framework	✅ success
previewjs	✅ success
qwik	✅ success
rakkas	✅ success
sveltekit	✅ success
vite-plugin-ssr	✅ success
vite-plugin-react	✅ success
vite-plugin-react-pages	❌ failure
vite-plugin-react-swc	✅ success
vite-plugin-svelte	✅ success
vite-plugin-vue	❌ failure
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

patak-dev · 2023-02-24T15:48:37Z

ecosystem-ci looks good now, fails are the same as current ones against main and vite-plugin-vue needs to update the snapshots

sapphi-red · 2023-02-25T08:20:27Z

Apparently I'm not allowed to run the ecosystem CI laughing Maybe someone else can run kick it off. @dominikg I wonder if we could loosen permissions to allow anyone with triage rights on GitHub to run it?

unfortunately not that simple, to trigger a run you need write access to ecosystem-ci repo (until the generic version is in place that can offer this via custom run triggers - or someone upgrades vite org to enterprise where you have more finegrained permission controls)

Because we trigger the CI by app, I guess just adding the username here might work.

vite/.github/workflows/ecosystem-ci-trigger.yml

Lines 18 to 31 in 3d3788d

    
                       const allowedUsers = new Set([ 
        
                         'yyx990803', 
        
                         'patak-dev', 
        
                         'antfu', 
        
                         'sodatea', 
        
                         'Shinigami92', 
        
                         'aleclarson', 
        
                         'bluwy', 
        
                         'poyoho', 
        
                         'sapphi-red', 
        
                         'ygj6', 
        
                         'Niputi', 
        
                         'dominikg' 
        
                       ])

bmeurer · 2023-02-27T08:23:08Z

Alright, thanks folks. So this is ready to land then?

patak-dev · 2023-02-27T08:39:47Z

Yes, let's merge it so we can test it during the 4.2 beta period

sapphi-red

I'm late but looks good to me 👍

sheremet-va · 2023-04-06T09:34:50Z

@patak-dev this broke Vitest's typechecking feature (it relies on mapConsumer.generatedPositionFor, which depends on the source path). I am not sure why this wasn't caught by the ecosystem-ci since we have a test for it. Is it possible to add an option to keep sources as an absolute path?

bmeurer · 2023-04-06T09:56:28Z

@sheremet-va That doesn't sound reliable. Couldn't vitest simply resolve relative entries in sources according to the source map spec? Just like the debuggers and other tools do?

sheremet-va · 2023-04-06T09:58:24Z

@sheremet-va That doesn't sound reliable. Couldn't vitest simply resolve relative entries in sources according to the source map spec? Just like the debuggers and other tools do?

I am not sure what you mean by "other tools". esbuild, for example, returns absolute paths.

sheremet-va · 2023-04-06T10:03:59Z

I am not sure what you mean by "other tools". esbuild, for example, returns absolute paths.

And it doesn't seem like Vite returns "sourceRoot" that can be used to identify the actual path.

bmeurer · 2023-04-06T10:38:58Z

This is not about source map generators; for those it's perfectly valid to put relative paths (or just file names) into the sources.

This is about source map consumers, and vitest is one of those consumers, just like debuggers (i.e. browser devtools) and other tools like bundlers or post-mortem stack processors; for consumers the Source Map Revision 3 Proposal states how to resolve sources entries:

If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).

sourceRoot is only a prefix that's prepended to all sources, but that's not where the producers should put absolute paths either. The consuming tools need to resolve the sources (after prepending sourceRoot) relative to the source map. In case of vite, let's say you have an output file /Users/foo/project/file.js which references input.ts. Then the consumer resolves this to /Users/foo/project/input.ts.

sheremet-va · 2023-04-06T10:55:06Z

In case of vite, let's say you have an output file /Users/foo/project/file.js which references input.ts. Then the consumer resolves this to /Users/foo/project/input.ts.

Vite's transformRequest doesn't return ID's absolute path, so it is impossible to determine the filesystem path for sources. It can also return virtual modules that don't have actual paths. Currently, Vitest tries to guess the path based on root and base, but it is not reliable. I can call transformRequest with an alias and it will return the processed result, but we cannot "reliably" consume the map if entries are relative.

Another thing is that transformRequest sometimes returns a map with entries relative to root, if ssr: true flag is present, so we cannot reliably get fs path even if we knew the absolute path of the module.

sheremet-va · 2023-04-06T10:57:26Z

In the end, Vitest modifies all maps to have absolute sources because this is how other tools consume it in Node.js environment, as far as I can see (e.g., source-map-support)

bmeurer · 2023-04-06T10:59:10Z

Without having the specific context of vitest, my reading of this would be that vite should supply the necessary information for tools to properly resolve sources entries.

sheremet-va · 2023-04-06T11:06:12Z

If the sources are not absolute URLs after prepending of the “sourceRoot”, the sources are resolved relative to the SourceMap (like resolving script src in a html document).

Also, as far as I understood this doesn't rule out the fact that source map's "sources" can be absolute paths. If there is no sourceRoot:

const sourceMapUrl = '/absolute`
const map = {
  sourceRoot: '',
  sources: ['entry.js']
  ...
}

resolve(sourceMapUrl, map.sources[0]) === '/absolute/entry.js'

const sourceMapUrl = '/absolute`
const map = {
  sourceRoot: '',
  sources: ['/absolute/entry.js']
  ...
}

map.sourceRoot + map.sources[0] // already absolute, skip next line
resolve(sourceMapUrl, map.sources[0]) === '/absolute/entry.js'

bmeurer · 2023-04-11T04:51:44Z

The current specification does not rule that out, yes.

I also don't want to block fixing the vitest feature. It's of course up to @patak-dev to decide whether having a different mode for sources when running with vitest is acceptable. My hunch is that it'd be better / more maintainable long-term to consistently use filenames (or worst case relative paths) in sources through all of vite to reduce the number of edge cases.

bmeurer force-pushed the bug/1411596_esbuild_sources_relative_paths branch 2 times, most recently from c135fca to b81a561 Compare February 16, 2023 13:50

bmeurer force-pushed the bug/1411596_esbuild_sources_relative_paths branch from b81a561 to 0bad9e6 Compare February 16, 2023 14:21

bmeurer changed the title ~~fix: Use relative paths in sources for esbuild~~ fix: use relative paths in sources for esbuild Feb 16, 2023

danielroe mentioned this pull request Feb 17, 2023

Generated Source Map (TS) nuxt/nuxt#15523

Closed

patak-dev previously approved these changes Feb 17, 2023

View reviewed changes

sapphi-red previously approved these changes Feb 17, 2023

View reviewed changes

patak-dev added this to the 4.2 milestone Feb 17, 2023

This was linked to issues Feb 17, 2023

Remove absolute local path to a component from source maps #2967

Closed

Typescript sourcemaps resolve to absolute full path when running live #4657

Closed

bmeurer added a commit to bmeurer/vite-plugin-vue that referenced this pull request Feb 21, 2023

fix: use relative paths for sources in Vue compiler

c38109a

Ref: vitejs/vite#12079

bmeurer dismissed stale reviews from sapphi-red and patak-dev via bfca7dc February 23, 2023 21:44

bmeurer force-pushed the bug/1411596_esbuild_sources_relative_paths branch from 0bad9e6 to bfca7dc Compare February 23, 2023 21:44

bmeurer changed the title ~~fix: use relative paths in sources for esbuild~~ fix: use relative paths in sources for transformed source maps Feb 23, 2023

danielroe mentioned this pull request Feb 23, 2023

feat: add sourcemapIgnoreList configuration option #12174

Merged

9 tasks

bmeurer force-pushed the bug/1411596_esbuild_sources_relative_paths branch from bfca7dc to f58ebde Compare February 23, 2023 21:53

patak-dev reviewed Feb 24, 2023

View reviewed changes

bmeurer force-pushed the bug/1411596_esbuild_sources_relative_paths branch from 1c53299 to 172c3e3 Compare February 24, 2023 12:54

patak-dev approved these changes Feb 25, 2023

View reviewed changes

patak-dev merged commit bcbc582 into vitejs:main Feb 27, 2023

bmeurer deleted the bug/1411596_esbuild_sources_relative_paths branch February 27, 2023 08:41

danielroe added a commit to danielroe/vite that referenced this pull request Feb 27, 2023

refactor: combine with loop from vitejs#12079

61bcf54

sapphi-red reviewed Feb 28, 2023

View reviewed changes

sapphi-red mentioned this pull request Feb 28, 2023

ci: allow collaborators to trigger ecosystem-ci #12233

Merged

4 tasks

patak-dev added a commit to vitejs/vite-plugin-vue that referenced this pull request Mar 1, 2023

chore: skip sourcemap tests after vitejs/vite/pull/12079

8e65d2e

patak-dev mentioned this pull request Mar 1, 2023

chore: skip sourcemap tests after vitejs/vite/pull/12079 vitejs/vite-plugin-vue#119

Merged

4 tasks

patak-dev added a commit to vitejs/vite-plugin-vue that referenced this pull request Mar 1, 2023

chore: skip sourcemap tests after vitejs/vite/pull/12079 (#119)

aa96ae4

patak-dev mentioned this pull request Apr 11, 2023

feat: server.sourcemapRelativeSources config option #12822

Closed

4 tasks

sapphi-red mentioned this pull request May 13, 2023

fix(ssr): stacktrace uses abs path with or without sourcemap #12902

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use relative paths in `sources` for transformed source maps #12079

fix: use relative paths in `sources` for transformed source maps #12079

bmeurer commented Feb 16, 2023 •

edited

Loading

benmccann commented Feb 16, 2023

patak-dev commented Feb 16, 2023

vite-ecosystem-ci bot commented Feb 16, 2023 •

edited

Loading

patak-dev commented Feb 17, 2023

sapphi-red left a comment •

edited

Loading

bmeurer commented Feb 17, 2023

sapphi-red commented Feb 17, 2023

bmeurer commented Feb 19, 2023

sapphi-red commented Feb 19, 2023

bmeurer commented Feb 20, 2023

bmeurer commented Feb 20, 2023

bmeurer commented Feb 21, 2023

bmeurer commented Feb 23, 2023

bmeurer commented Feb 23, 2023

patak-dev Feb 24, 2023

bmeurer Feb 24, 2023

sapphi-red Feb 25, 2023

patak-dev commented Feb 24, 2023

vite-ecosystem-ci bot commented Feb 24, 2023 •

edited

Loading

patak-dev commented Feb 24, 2023

sapphi-red commented Feb 25, 2023

bmeurer commented Feb 27, 2023

patak-dev commented Feb 27, 2023

sapphi-red left a comment

sheremet-va commented Apr 6, 2023 •

edited

Loading

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023 •

edited

Loading

bmeurer commented Apr 11, 2023

fix: use relative paths in sources for transformed source maps #12079

fix: use relative paths in sources for transformed source maps #12079

Conversation

bmeurer commented Feb 16, 2023 • edited Loading

Description

Additional context

What is the purpose of this pull request?

Footnotes

benmccann commented Feb 16, 2023

patak-dev commented Feb 16, 2023

vite-ecosystem-ci bot commented Feb 16, 2023 • edited Loading

patak-dev commented Feb 17, 2023

sapphi-red left a comment • edited Loading

Choose a reason for hiding this comment

bmeurer commented Feb 17, 2023

sapphi-red commented Feb 17, 2023

bmeurer commented Feb 19, 2023

sapphi-red commented Feb 19, 2023

bmeurer commented Feb 20, 2023

bmeurer commented Feb 20, 2023

bmeurer commented Feb 21, 2023

bmeurer commented Feb 23, 2023

bmeurer commented Feb 23, 2023

patak-dev Feb 24, 2023

Choose a reason for hiding this comment

bmeurer Feb 24, 2023

Choose a reason for hiding this comment

sapphi-red Feb 25, 2023

Choose a reason for hiding this comment

patak-dev commented Feb 24, 2023

vite-ecosystem-ci bot commented Feb 24, 2023 • edited Loading

patak-dev commented Feb 24, 2023

sapphi-red commented Feb 25, 2023

bmeurer commented Feb 27, 2023

patak-dev commented Feb 27, 2023

sapphi-red left a comment

Choose a reason for hiding this comment

sheremet-va commented Apr 6, 2023 • edited Loading

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

sheremet-va commented Apr 6, 2023

bmeurer commented Apr 6, 2023

sheremet-va commented Apr 6, 2023 • edited Loading

bmeurer commented Apr 11, 2023

fix: use relative paths in `sources` for transformed source maps #12079

fix: use relative paths in `sources` for transformed source maps #12079

bmeurer commented Feb 16, 2023 •

edited

Loading

vite-ecosystem-ci bot commented Feb 16, 2023 •

edited

Loading

sapphi-red left a comment •

edited

Loading

vite-ecosystem-ci bot commented Feb 24, 2023 •

edited

Loading

sheremet-va commented Apr 6, 2023 •

edited

Loading

sheremet-va commented Apr 6, 2023 •

edited

Loading