Feature/sourcemap preprocessor support

[halfnelson]

What

Adds support for the handling of source maps generated by preprocessors.
This is a rebase of parts of #5428 (by @milahu) to make it easier to review.

Why

Continuing the ongoing effort for a solid Typescript story for Svelte, The Typescript pre-processor and IDE support are good enough for prime time. The main letdown is the debug story. Svelte component source maps ignore the preprocessing that takes place making it impossible to do meaningful debugging via the web dev tools.

How

We use https://github.com/ampproject/remapping a small and synchronous source map combiner (Based on feedback on #1863)

Some manual source map merging is done in the context of Script and Style preprocessor results using a small set of utility functions developed for that purpose.

Sample level tests have been written to prove the flow.

Next Steps

The bundler plugins/loaders will need to be made aware of the sourcemap compiler option and pass in any preprocessor results (e.g. sveltejs/rollup-plugin-svelte#140)

Apply the developer mode warnings and encode/decode optimizations from #5428

[halfnelson]

@benmccann @dummdidumm I took a stab at a refactor of preprocess/index.ts to make it more understandable, but I am reluctant to commit it to this branch just yet, in case the changes are viewed as too drastic.

https://github.com/halfnelson/svelte/blob/1217c5b24a3499db2ed302436050a82a4419b2f6/src/compiler/preprocess/index.ts
It passes lint and all the tests, I feel it is easier to understand although I know that is subjective. Should I merge it into this PR?

[milahu]

nice one!

line 132

processed_tags.sort((a,b) => a.tag.offset > b.tag.offset ? 1 : (a.tag.offset == b.tag.offset ? 0 : -1));

isnt that just

processed_tags.sort((a,b) => a.tag.offset - b.tag.offset);

to avoid temporary variables, the methods from_source, from_processed and concat could be merged
so this

const leading_content = StringWithSourcemap.from_source(filename, source.slice(last_end, tag.offset) + open_tag, locator(last_end));
const processed_content = StringWithSourcemap.from_processed(processed.code, processed.map, locator(tag.content_offset));
const trailing_content = StringWithSourcemap.from_source(filename, close_tag, locator(tag.content_offset + tag.content.length - 1));
out.concat(leading_content).concat(processed_content).concat(trailing_content);

becomes this

out.add_source(filename, source.slice(last_end, tag.offset) + open_tag, locator(last_end));
out.add_processed(processed, locator(tag.content_offset));
out.add_source(filename, close_tag, locator(tag.content_offset + tag.content.length - 1));

(etc)

locator should be locate or getloc (imperative fn name)

target.applyPreprocessorOutput(await fn({
	content: target.source,
	filename: target.filename
}));

could be less ugly, if we rename target.source to target.content and call fn(target)
or preprocessor(target)

filter(t => t != null) aka filter(Boolean)

return target.source; should be return this.code; no?

[halfnelson]

isnt that just

processed_tags.sort((a,b) => a.tag.offset - b.tag.offset);

Good point :)

to avoid temporary variables, the methods from_source, from_processed and concat could be merged

I guess this is a personal preference. I don't mind the extra variables here as I feel that they help make the core action more readable out.concat(leading_content).concat(processed_content).concat(trailing_content)

locator should be locate or getloc (imperative fn name)

Strongly agree. I will change it.

could be less ugly, if we rename target.source to target.content and call fn(target)
or preprocessor(target)

I don't mind being explicit when crossing a responsibility/interface boundary.

filter(t => t != null) aka filter(Boolean)

ok

return target.source; should be return this.code; no?

Good point, that will allow JS to collect the target object.

Thanks for solid review!

[halfnelson]

updated and ninja edited the original link

[benmccann]

yeah, it's a good change, but I think it probably is better to separate into two PRs to keep the review manageable

[Conduitry]

At long last, this has been released in 3.30.0 🎉 Thank you to the several people involved in the literal years since the original PR!

[milahu]

sweet : )

if someone wants to optimize this even more:
see my todo list in #5428

currently, the libraries code-red and remapping
always return encoded sourcemaps
which sometimes must be decoded, to merge sourcemaps

this encode + decode steps are unnecessary and expensive
in my benchmarks, this was the only optimization
which made a visible difference in compile time

the technical side of to problem is solved (patches are made)
what remains is the diplomatic side ....
aka politely kick some maintainer ass to merge the patch
(or make a fork project and release that via NPM)

to optimize the merging of sourcemaps, see
Rich-Harris/sourcemap-codec#82
currently, this is unnecessarily slow
cos we need binarySearch to find segments by column, as discussed here
ampproject/remapping#88
again, the technical side is trivial to solve

[benmccann]

Any improvements would be great! I'm happy to help review those and get them in

Btw, @dummdidumm reported the line numbers might be off by one in his tests. I haven't gotten time to really test much yet. I have a PR to add the feature to rollup-plugin-svelte here that you can use for testing: sveltejs/rollup-plugin-svelte#157

[milahu]

Btw, @dummdidumm reported the line numbers might be off by one in his tests.

a possible external cause:
decoded sourcemaps store zero-based lines and columns
mozilla's SourceMapConsumer returns one-based lines and zero-based columns

svelte/test/sourcemaps/index.ts

Lines 98 to 103 in 505eba8

	// use locate_1 with mapConsumer:
	// lines are one-based, columns are zero-based
	preprocessed.mapConsumer = preprocessed.map && await new SourceMapConsumer(preprocessed.map);
	preprocessed.locate = getLocator(preprocessed.code);
	preprocessed.locate_1 = getLocator(preprocessed.code, { offsetLine: 1 });

possible test bug:
the tests in test/sourcemaps/samples could be too simple
cos they only replace short tokens, not multiple lines,
so the line numbers never change

[milahu]

test bug

the function magic_string_preprocessor_result
must allow to generate decoded mappings
as needed by the test decoded-sourcemap

sample solution

export function preprocessor_result (
	filename: string,
	src: MagicString,
	map_options: any
) {
	map_options = Object.assign({
		// default options
		source: filename,
		hires: true,
		includeContent: false
	}, map_options || {});
	const get_map = map_options.skipEncode ? src.generateDecodedMap : src.generateMap;
	delete map_options.skipEncode;
	return {
		code: src.toString(),
		map: get_map(map_options)
	};
}

// sample use
return preprocessor_result(filename, src, { skipEncode: true });

(also, by convention in object-oriented-programming, the object always comes as first parameter, but thats "only" aesthetics)

(to make tests faster, result() should return decoded mappings by default, and only encode mappings when map_options = { encodeMappings: true } is set)