Fix some issues with preprocess source maps #5754
Conversation
There was a problem hiding this comment.
Awesome, thank you!
The basename-stuff got me thinking. Maybe we need another test to check that external sources behave as expected. So for example that this
<style lang="scss" src="./someexternal.scss"></style>
<h1>Hello world</h1>Is mapped correctly. So for example if there's a warning inside the transpiled css that there's no original position (because it does not exist inside the original Svelte file) and that the subsquent code is not affected, either.
src/compiler/preprocess/index.ts
Outdated
| return decoded_map.sources.findIndex(source => !source); | ||
| } | ||
| // different tools produce different sources | ||
| // (file name, relative path, absolute path) |
There was a problem hiding this comment.
can we add a test for these different cases?
There was a problem hiding this comment.
Added guess-source. I'm not sure if this is the right approach though... While it may workaround some cases, maybe it would be better to require correct maps from preprocessors?
There was a problem hiding this comment.
I do think it would be a good idea to have preprocessors producing more consistent sourcemaps either way even if we didn't require them here. Requiring them sounds like a reasonable idea though. Do you know which ones are producing these different paths and what the preferred version is? It could be helpful to file an issue against https://github.com/sveltejs/svelte-preprocess with these details
There was a problem hiding this comment.
I had issue with postcss transformer. The first thing that comes to mind - it should pass "to" parameter to postcss (equal to "from"). Will check and report later.
|
Removed source guessing. |
|
I think we can bump magic-string, thoughts @Conduitry ? What was the reason to remove source guessing? |
The more I looked at it, the more wrong this feature seemed to me. For example, if there were two sources I think there shouldn't be such workaround here and it's up to preprocessors to produce correct sources. |
|
Yeah, I would say to go ahead and bump |
|
Resulted in failed |
|
If upgrading |
|
@dmitrage but seems like politeness is better than competence .. edit: my bad, i was too dumb to actually release my pending comments |
|
Insults don't help. And what comments where? |
I want to ensure that all changes are correct, maybe add some new tests. Run some benchmarks on real code. And cleanup a bit. I also want to finish with TODO in PR description and maybe bump |
|
Sorry for the delay and blocking others, had less spare time than expected. I decided to postpone some things so ready for final review. I renamed Notes about postponed:
I won't be able to work on these in few days (not related to cbp2077 release as one could thought :-P) and will file issues a bit later so that somebody could pick it. |
| if (seg[2]) seg[2] += offset.line; | ||
| // shift only segments that belong to component source file | ||
| if (seg[1] === source_index) { // also ensures that seg.length >= 4 | ||
| // shift column if it points at the first line | ||
| if (seg[2] === 0) { | ||
| seg[3] += offset.column; | ||
| } | ||
| // shift line | ||
| seg[2] += offset.line; | ||
| } |
There was a problem hiding this comment.
would be nice to have a test for "only shift lines of one source file"
probably you have good reasons for this change, but its not yet transparent
if your solution is right, then we get another problem:
what about basename collisions?
a/index.svelte + b/index.svelte --> c/index.svelte
the sourcemap.names array is not sorted, so its not predictable
so there is no (simple) way to choose the right index.svelte file
lazy solution: print a warning.
"dear user, sorry, but our compiler knows files only by their basename .."
a more radical solution: instead of file names, use file paths.
these are always relative to the project root
all our plugins would have to preserve these paths
There was a problem hiding this comment.
test for "only shift lines of one source file"
sourcemap-offsets should cover this. Or am i misunderstanding you?
what about basename collisions?
I think that I have a different vision of the build pipeline:
- Preprocess
- We assume that preprocess src === dst and map file is placed in same dir. We use
basenamefor component in sources since it should be relative to the sourcemap file. - We use
basenamefor chunks that we don't pass to preprocessors (StringWithSourcemap.from_source) - We pass full
filenameto preprocessors and expect sources to containbasenamewhen referring component being preprocessed andrelative to filename pathsfor external inclusions. For example, expect["index.svelte", "../b/index.svelte"]when transforminga/index.svelte(no collision here).
- Compile
- We pass
filename,outputFilenameandcssOutputFilename. Based on these, svelte compiler creates source map with sources as relative paths. - Then we combine js and css maps from compiler with preprocessed map so that segments point at locations in original source code and sources in resulted maps are relative from output map file path to original source files.
- Bundle (optional)
- Should work as usual since we input compiled files with maps pointing directly at original sources.
Is there anything missing, you disagree with or that I should describe more clearly? I'd prefer not to land something cryptic or/and broken :)
P.S. Just out of interest, is it synthetic or do you know setups when preprocess step combines multiple components into one?
There was a problem hiding this comment.
sourcemap-offsetsshould cover this.
yes! sorry. ("dear mila, please just read the source")
We use
basenamefor component in sources since it should be relative to the sourcemap file.
yes .. i wrongly assumed that the basename-function is applied to the preprocessor-result
while its only applied to the svelte.preprocess input filename
We pass full
filenameto preprocessors and expect sources to containbasenamewhen referring component being preprocessed
this is my problem with this solution ..
why do we always convert file paths to basenames?
(always, without giving an option to the user)
short answer: for consistency with the existing svelte.compile
good news: i am happy with your patch : )
long answer:
[email protected] + [email protected]
svelte.preprocess and svelte.compile receive relative paths
options = { filename: 'src/App.svelte' }
[email protected] + [email protected]
svelte.preprocess and svelte.compile receive absolute paths
options = { filename: '/home/user/project/src/App.svelte' }
why do we always convert file paths to basenames?
the actual reason here is MagicString, who turns file paths into basenames:
// svelte/src/compiler/compile/render_dom/index.ts
const css = component.stylesheet.render(
options.filename, // file path
!options.customElement
);
// svelte/src/compiler/compile/css/Stylesheet.ts
// Stylesheet.render
return {
code: code.toString(),
map: code.generateMap({
includeContent: true,
source: this.filename, // file path
file
})
};
// --> result.map.sources: basename
// magic-string/src/MagicString.js
// MagicString.generateDecodedMap
return {
// path to basename
file: options.file ? options.file.split(/[/\\]/).pop() : null,
sources: [options.source ? getRelativePath(options.file || '', options.source) : null],
sourcesContent: options.includeContent ? [this.original] : [null],
names,
mappings: mappings.raw
};so the path-to-basename conversion is done for consistency with other sourcemaps, generated by svelte.compile
There was a problem hiding this comment.
why do we always convert file paths to basenames?
basename is just a result of computing relative path from file to itself since I assumed that we "put" results of preprocess into the same directory where source code lives.
If someone needs more control we could add additional option outputFilename to preprocess so that specific relative paths are generated instead. For example, take ./src/index.svelte, preprocess it and put into ./preprocessed/index.svelte, and then pass those files to svelte.compile independently. In this case, instead index.svelte (basename) we would have our component referred by index.svelte.map as ../src/index.svelte (relative to map file location - from {cwd}/preprocessed/index.svelte.map to {cwd}/src/index.svelte).
But I'm not sure that such feature is really needed and worth complication both in usage and implementation since usually preprocess is done in-memory and can be treated as atomic operation with compile. I might be wrong here though. But then it can be implemented separately.
So I would say it's more about simplicity, not consistency.
There was a problem hiding this comment.
can we add a test for the basename conversion? something like ....
import { magic_string_bundle } from '../../helpers';
const external_filename = 'external_code.css';
export const external_code = `/* external code start */
span {
--external_code-var: 1px;
}
/* external code end */`;
const component_filepath = 'src/input.svelte';
const component_basename = 'input.svelte';
export default {
js_map_sources: [component_basename],
css_map_sources: [component_basename, external_filename],
preprocess: [
{
style: ({ content, filename }) => {
return magic_string_bundle([
{ code: external_code, filename: external_filename },
{ code: content, filename }
]);
}
}
],
options: {
filename: component_filepath
}
};it can be implemented separately.
yes .. low priority stuff
what i meant was:
optionally, svelte should "simply" preserve all file paths (relative or absolute)
then its the bundler's job (rollup, webpack) to feed pretty file paths to svelte
for example
src/App.svelte should be preserved
when src/App.svelte includes ./style/included.css
then its resolved to src/style/included.css
makes easier the debugging of larger projects with many files + folders
There was a problem hiding this comment.
makes easier the debugging of larger projects with many files + folders
Sorry, will response ASAP.
There was a problem hiding this comment.
@milahu I think debugging should be fine. Bundler (or dev server) gives us filename, usually absolute or relative to cwd. We return compiled code and source map with sources relative to given filename (as they are stored in map). I think it's up to bundler/server to correctly resolve sources in resulting map. I tried some simple cases with rollup and it seemed to work as expected. Would appreciate if you could help me finding some repro with broken debugging experience so I could play with it if you have some time :)
There was a problem hiding this comment.
well thats easy ..
https://github.com/milahu/svelte-template--reproduce-basename-vs-filepath
actual result:
all paths are reduced to basename
(see my other comment, this is done by MagicString.generateDecodedMap)
expeted result:
optionally show the full file path, for example src/menu/App.svelte
There was a problem hiding this comment.
I made the filename relative to cwd in rollup-plugin-svelte in sveltejs/rollup-plugin-svelte#131 because that's what I had seen in the couple example source maps I looked at. Assuming that's the correct behavior (I haven't fully followed the full discussion here), we could make that change to webpack's svelte-loader as well.
There was a problem hiding this comment.
I cloned it, launched and console logged the same. I also did some tweak after, will describe in my next comment. But if I hover App.svelte:2, I see full url to correct source file. Clicking also navigates to where I would expect.
|
While investigating https://github.com/milahu/svelte-template--reproduce-basename-vs-filepath I made some changes:
Next build and... INCORRECT MAPPINGS... After some investigation I located that the problem was in generating One thing that could be tweaked - check if code returned from preprocessor equals code passed to it and there is no source map (i.e. nothing changed). But not sure about this - preprocessor can return |
are these two connected? how is that behavior expected? |
|
Original template: no preprocess, rollup generates sourcemap from sources, console.log calls are mapped to original components. P.S. Looked at |
this means that see StringWithSourcemap.from_source to generate an identity map edit: in StringWithSourcemap.from_source we have the same old "shift columns in first line" logic // shift columns in first line
const segment_list = map.mappings[0];
for (let segment = 0; segment < segment_list.length; segment++) {
segment_list[segment][3] += offset.column;
}not sure if thats correct, a test would be nice .. |
We pass preprocessed result to
Will re-check but since we have one-to-one mapping I expect it to work (i.e. only and all segments on first generated line are pointing at segments on first original line). |
|
Right now I have no plans to change anything here. But there is #5793 and let me talk about it a bit. To get initially scared, I ran some preprocess tools with URL fake file path on win32. TL;DR:
console.log(
require('typescript').transpileModule('123', {
fileName: 'file:///somewhere/file.ts',
outFile: 'file:///somewhere/file.js',
compilerOptions: { sourceMap: true }
}).sourceMapText
);
// => {"version":3,"file":"file.js","sourceRoot":"","sources":["file.ts"],"names":[],"mappings":"AAAA,GAAG,CAAA"}
console.log(
require('postcss')()
.process('div {}', {
from: 'file:///somewhere/file.pcss',
to: 'file:///somewhere/file.css',
map: {
inline: false
}
})
.map.toString()
);
// => {"version":3,"sources":["file:///somewhere/file.pcss"],"names":[],"mappings":"AAAA,KAAK","file":"file:///somewhere/file.css","sourcesContent":["div {}"]}
console.log(
require('sass')
.renderSync({
file: 'file:///somewhere/file.scss',
outFile: 'file:///somewhere/file.css',
data: 'div { color: red; }',
sourceMap: true,
sourceMapEmbed: false
})
.map.toString()
);
// => {"version":3,"sourceRoot":"","sources":["file:///D:/DM/Development/Local/check-tools/file:/somewhere/file.scss"],"names":[],"mappings":"AAAA;EAAM","file":"file.css"}We pass We could try something like:
... And there could be multiple external sources returned by preprocessor... Should they also have So it quickly gets too out of scope of current PR. This one is mostly about offsets in preprocessed source code. I think that a dedicated PR with focus on correctly computing sources in both preprocess and compile steps would be a better place to make decisions. Edit: read discussion again and maybe there is no need to care about passing URL to preprocessors at all. |
|
The SASS one seems too crazy for us to deal with it and we should probably file a bug against it and try to get it fixed at the source |
|
Btw, if you guys have any more interest in carrying on this work, I saw a couple TODOs in the code where CSS source maps are not fully implemented svelte/src/runtime/internal/ssr.ts Line 120 in ddf1321 |
Hi! I was trying to recalculate locations of warnings using sourcemap generated from preprocess and I think that I've found some issues. Here is the list of changes I came up to:
I would appreciate feedback on this. Pinging @milahu and @dummdidumm based on recent activity on linked issue but everyone is welcome :) Thanks!