add source map support for preprocessors #5015

halfnelson · 2020-06-14T12:58:19Z

What
Adds support for the handling of source maps generated by preprocessors.

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.

antony · 2020-07-11T20:18:09Z

@halfnelson does this PR supersede #1863 ?

halfnelson · 2020-07-11T22:43:00Z

@antony yep, it attempts to meet Rich's criteria "it'd be great if we could avoid using source-map"

halfnelson · 2020-07-11T22:47:03Z

The downside is it isn't as clean as RedHatters awesome contribution, so happy for suggestions. I tend to stop at the "it works now" stage

benmccann · 2020-08-03T13:39:26Z

@halfnelson thanks for this! really excited about it. One thing I noticed from a quick peek at this PR is that a lot of the new code uses spaces. Could you convert it to tabs to match the existing code?

benmccann · 2020-08-19T21:39:51Z

package.json

@@ -91,7 +92,8 @@
    "source-map-support": "^0.5.13",
    "tiny-glob": "^0.2.6",
    "tslib": "^1.10.0",
-    "typescript": "^3.5.3"
+    "typescript": "^3.5.3",
+    "sourcemap-codec": "^1.4.8"


can you move this up a few lines so that it's in alphabetical order?

benmccann · 2020-08-19T21:43:40Z

src/compiler/compile/Component.ts

@@ -324,6 +325,35 @@ export default class Component {
 			js.map.sourcesContent = [
 				this.source
 			];
+
+			if (compile_options.sourceMap) {


I would probably call this option sourcemap (all lowercase) in order to match the rollup option name. I think it'd be easier to remember what to set if they're the same

benmccann · 2020-08-19T21:45:52Z

src/compiler/preprocess/index.ts

+		let out: GeneratedStringWithMap;
+		let last_end = 0;
+		for (const { offset, length, replacement } of replacements)
+		{


I think this should probably be on the previous line for consistency with the existing code

benmccann · 2020-08-19T21:47:53Z

src/compiler/utils/string_with_map.ts

+        if (this.generated.length == 0) return other;
+        if (other.generated.length == 0) return this;
+
+        //combine sources


there's a mix here with there sometimes being a space after // and sometimes not. I'd probably always include a space for consistency

milahu · 2020-09-05T14:08:16Z

@halfnelson said

I tend to stop at the "it works now" stage

thanks for the patch! lets get this done ....

shamelessly rebased to current master:
https://github.com/milahu/svelte/tree/feature/preprocessor-sourcemaps
(took a while until i found git pull --rebase)

conflict was: new version allows self-closing <script/> and <style/> tags

/<!--[^]*?-->|<script(\s[^]*?)?(?:>([^]*?)<\/script>|\/>)/gi,
/<!--[^]*?-->|<style(\s[^]*?)?(?:>([^]*?)<\/style>|\/>)/gi,

made a mistake on rebase:
failed to use the new <style> regex, fixed in commit 'prettify code 3'
assuming the commits are squashed

applied all suggestions by @benmccann

further prettified the code (except tests), added comments, .... see commit messages

all tests pass

should i make a new PR?

milahu · 2020-09-11T09:37:37Z

push - what can i do to get this merged?

milahu · 2020-09-18T15:52:58Z

dear maintainers - @benmccann @Conduitry etc

what are you waiting for?
the request author @halfnelson has obviously lost interest in the issue

i fixed all suggestions by @benmccann in my fork
should i make a new PR?

antony · 2020-09-18T16:22:16Z

@milahu we're not waiting for anything. We're extremely busy, using the little free time we have at the moment to work on Sapper.

Please open a new PR, mention this one and we will review it in due course.

milahu

milahu · 2020-09-13T15:43:18Z

src/compiler/utils/string_with_map.ts

+        //shift the first line of the second mapping by the number of columns in the last line of the first
+        const end = get_end_location(this.generated);
+        const col_offset = end.column + 1;
+        const first_line = other_mappings.length == 0 ? [] : other_mappings[0].map(seg => {
+            const new_seg = seg.slice() as MappingSegment;
+            new_seg[0] = seg[0] + col_offset;
+            return new_seg;
+        });


micro optimization:
we can skip column-shifting if last line of first part is empty
= first_source_str.slice(-1) == '\n'

what value has end.column in that case?
i guess its not -1 but rather line_length - 1
so the old column-shifting would be wrong

done
end.column is zero if last line is empty (pass)
added micro optimization - skip colum shifting if col_offset is zero

milahu · 2020-09-18T19:03:28Z

src/compiler/preprocess/index.ts

+		return map;
+	}
+
+	async function replace_async(str: string, re: RegExp, func: (...any) => Promise<GeneratedStringWithMap>): Promise<GeneratedStringWithMap> {


this fn replace_async was moved into fn preprocess to use the global variable filename.
should we keep this? or change the fn signature of replace_async
to reduce line diffs of patch and to get a pure fn (clean interface)

done
moved fn replace_async out of fn preprocess, added filename and get_location to fn signature

halfnelson · 2020-10-25T10:43:31Z

This lives on in #5428

add source map support for preprocessors

6c0a867

kaisermann mentioned this pull request Jun 19, 2020

How to generate sourcemaps? sveltejs/svelte-preprocess#140

Closed

benmccann mentioned this pull request Jul 2, 2020

Handle sourcemaps sveltejs/rollup-plugin-svelte#44

Merged

antony mentioned this pull request Jul 11, 2020

Consume and generate sourcemaps #1863

Closed

dummdidumm mentioned this pull request Jul 22, 2020

[Proposal] Run js expressions in markup template through Svelte script preprocessor code #4701

Closed

benmccann mentioned this pull request Jul 22, 2020

Source map line numbers are wrong when using typescript #5182

Closed

benmccann reviewed Aug 19, 2020

View reviewed changes

benmccann mentioned this pull request Sep 14, 2020

unused css highlighting is always messed up sveltejs/language-tools#540

Closed

milahu approved these changes Sep 19, 2020

View reviewed changes

milahu mentioned this pull request Sep 19, 2020

Feature/preprocessor sourcemaps #5428

Closed

halfnelson closed this Oct 25, 2020

halfnelson mentioned this pull request Oct 25, 2020

Refactor SourceMap and Preprocessor tests #5583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add source map support for preprocessors #5015

add source map support for preprocessors #5015

halfnelson commented Jun 14, 2020 •

edited

Loading

antony commented Jul 11, 2020 •

edited

Loading

halfnelson commented Jul 11, 2020

halfnelson commented Jul 11, 2020

benmccann commented Aug 3, 2020

benmccann Aug 19, 2020

benmccann Aug 19, 2020

benmccann Aug 19, 2020

benmccann Aug 19, 2020

milahu commented Sep 5, 2020 •

edited

Loading

milahu commented Sep 11, 2020

milahu commented Sep 18, 2020

antony commented Sep 18, 2020

milahu left a comment •

edited

Loading

milahu Sep 13, 2020

milahu Sep 19, 2020

milahu Sep 18, 2020

milahu Sep 19, 2020

halfnelson commented Oct 25, 2020

add source map support for preprocessors #5015

add source map support for preprocessors #5015

Conversation

halfnelson commented Jun 14, 2020 • edited Loading

antony commented Jul 11, 2020 • edited Loading

halfnelson commented Jul 11, 2020

halfnelson commented Jul 11, 2020

benmccann commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

milahu commented Sep 5, 2020 • edited Loading

milahu commented Sep 11, 2020

milahu commented Sep 18, 2020

antony commented Sep 18, 2020

milahu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halfnelson commented Oct 25, 2020

halfnelson commented Jun 14, 2020 •

edited

Loading

antony commented Jul 11, 2020 •

edited

Loading

milahu commented Sep 5, 2020 •

edited

Loading

milahu left a comment •

edited

Loading