fix(vitePreprocess): use relative paths in sourcemap sources and output css dependencies

[dominikg]

based on the work done in #621 by @bfanger , but uses a unit test and more generic implementation.

[bfanger]

The removeSuffix implementation might be too aggressive in changing the sources and a hair slower, compared to the exact match implementation we had before, but the chances a developer is creating .svelte.scss files (and using the same component name in a different folder and importing these) is very very low.

The color: red; is missing a indentation level (doesn't matter for the test but it triggered my prettier senses)

Should we add standalone unittests for the removeLangSuffix utils?, to test both the regular:

SourceMap {
  version: 3,
  file: '/Volumes/Sites/svelte-project-template/src/lib/components/Hello/Hello.svelte.scss',
  mappings: 'AACE;EACE;EACA;EACA;EACA;EACA;EACA;EACA;EACA;;AAEA;EACE',
  names: [],
  sourceRoot: undefined,
  sources: [
    '/Volumes/Sites/svelte-project-template/src/lib/components/Hello/Hello.svelte.scss'
  ]
}

and the variant with a sourceRoot:

{
  version: 3,
  sourceRoot: '/Volumes/Sites/vite-plugin-svelte/packages/e2e-tests/css-dev-sourcemap/src',
  sources: [ 'App.svelte.scss' ],
  names: [],
  mappings: 'AAEE;EACC',
  file: 'App.svelte.scss'
}

or would that be overkill?

[dominikg]

no, the additional unit tests for the util sound like a good idea.

While trying to fix the windows fail (god those fs paths on windows are a pita) i've come across the fact that vitePreprocess can also emit file:/// style source entries, which makes me wonder if we also have to strip that before turning something relative. GAH!

[dominikg]

Oh, and is a sourcesRoot is set, it would have to account for that when turning the path to a relative one.

Although having a sourcesRoot and then absolute entries in sources counts as a crime against tooling developers 👿

[dominikg]

The removeSuffix implementation might be too aggressive

The endsWith implementation was done because it's possible that vite already rewrites the path and then we would no longer strip the suffix. The case that someone has App.svelte and App.svelte.scss is a risk though. Maybe we can use a more custom suffix to avoid userspace collisions.

eg App.svelte.somerandom.scss or App.svelte?lang=.scss ?

cc @bluwy

[dominikg]

the unique suffix separator actually simplifies suffix removal and avoids the userspace collisions. 🥳

[dominikg]

Dear microsoft, please get rid of drive letters and use / . Thanks

[bfanger]

import { fileURLToPath } from "url"

is useful for handling file urls.

Yeah, working with files on windows is complicated. Had an issue a while back because some node ap'si return c: (lowercase) and some apis return C: (uppercase)

[dominikg]

sourceRoot can actually be a problem when converting to relative. We can't convert some and leave sourceRoot intact, that could end up with invalid paths when sourceroot is prepended to the relative output.

So if sourceRoot is set, we have to make sure all paths are converted to relative and then unset it? damn this stuff is messy

[bfanger]

What is the reason we do the conversion to relative? Can we assume the sources are correct if the map contains a sourceRoot?

[dominikg]

Unfortunately I don't think we can. It's possible that no tool used by vite currently emits sourcemaps with sourceRoot set, but if one did, it would have to be accounted for and if sourceRoot+source turned out to be an absolute path, that would be bad.

Always using relative paths from the location of the sourcemap (sibling of the file it maps) prevents leaking file system information like your home directory and is required for reproducible builds.

source paths are also used by the svelte compiler later on to merge sourcemaps with the additional work it is doing, so it is helpful if we only send one consistent output to that, not sometimes sourceroot, sometimes absolute paths, sometimes relative. Ideally it would be relative paths only all the time.

[bfanger]

I didn't think about sourcemap as an attack vector, but changing absolute /etc/passwd to relative ../../../../etc/password doesn't make it any safer. I think this is something for vite to check and ignore files that are outside its root.

[dominikg]

It's not about paths outside of vite root, thats a different problem.
Lets say we both have a project cloned. You use /home/bfanger/work/cool-project and I use /home/dominikg/dev/cool-project

In both locations, the sourcemaps produced by vite dev or vite build should be identical and independant of the root directory. This is also called reproducible/deterministic/hermetic sometimes

[bfanger]

I see, still think that's vite problem, not a vite-plugin issue.
I don't mind differences in dev and build, (vite doesn't even support css sourcemaps in build at the moment.)

When running a build, vite should always generate relative sourcemaps.

[dominikg]

@bmeurer - sorry to cold-tag you here but i'd love to pick your brain on a way to safely convert all file references in a sourcemap to relative paths, regardless of what they are.

This PR contains a function that achieves it in my opinion for outputs of vite's css transform, which surprisingly can even container file:/// style entries in sources.

One area where i am not 100% sure is how sourceRoot is to be prepended to sources. Do you have to add a / between the two or not? And do you always prepend it or only if the source isn't absolute already?

[bluwy]

FWIW, Vite uses path.resolve, but I'm also not sure because the sourcemap doesn't cover it. https://github.com/vitejs/vite/blob/62ebe28d4023c1f67578b1977edd3371f44f475a/packages/vite/src/node/server/sourcemap.ts#L41

[bmeurer]

@bmeurer - sorry to cold-tag you here but i'd love to pick your brain on a way to safely convert all file references in a sourcemap to relative paths, regardless of what they are.

This PR contains a function that achieves it in my opinion for outputs of vite's css transform, which surprisingly can even container file:/// style entries in sources.

One area where i am not 100% sure is how sourceRoot is to be prepended to sources. Do you have to add a / between the two or not? And do you always prepend it or only if the source isn't absolute already?

The specification isn't really clear here. I went with just path.relative() in vite before, which seems to work so far. I don't really have a generalized solution. Regarding sourceRoot, Chrome DevTools adds a / in between unless sourceRoot ends with / or the path starts with / (code).

[dominikg]

went with fileURLToPath after all to avoid encoded chars being a problem and also adding a single slash between sourceRoot and source. While not sure this is perfect, i assume this is as close as it gets within the spec.

sourceRoot is not prepended to file:/// style sources as that doesn't make any sense

[dominikg]

@bfanger readded your e2e test for dev sourcemap but sprouced it up with an scss import and changing the assertions from snapshot to decoding the sourcemap comment to see that files are there.

During all this I also realized we didn't pass on the dependencies, this has been added as well to allow better hmr experience when editing these @import'ed files.