Feature/preprocessor sourcemaps

[milahu]

continue request #5015 by @halfnelson

this patch got larger than expected. let me know if i should must make it smaller
most noise is in refactoring of tests

done - major changes

• bugfix in fn merge_tables (see end of comment)
• optimize: skip unnecessary encode+decode steps
  • between res.get_sourcemap() and remapping(sourcemap_list) - make preprocess 20% faster in benchmark
  • between svelte.preprocess and svelte.compile - waiting for add print option sourceMapEncodeMappings Rich-Harris/code-red#51 and add option decodeMappings to remapping ampproject/remapping#88
• use the loader interface of remapping to support any number of source files = convergent transform chains with multiple bundle steps

done - small changes

• fix suggestions by benmccann in request 5015
• rename variables
• rephrase comments
• indent with tabs
• micro optimize: skip column-shifting if offset is zero
• verified edge case: when concatting code segments, and when the first segment ends with a newline (empty line), then offset (for column-shifting) is zero
• move functions replace_async and get_replacement to global scope, make pure fn, avoid using global vars
• refactor sourcemap tests, move _preprocess.js to _config.js, rename variables, etc
• fix test preprocess/filename. preprocessors.filename is ignored if options.filename is set
• handle sourcemap.names, dont just return empty array
• add test: sourcemap with names field, verify merging and remapping of map.names
  names is an array of original symbol names that were changed in the transform (sample: terser.minify)
• micro optimize: use mutable data, much faster than immutable
  • replace array.concat with array.push
  • avoid array1.push(...array2) which is limited by max call stack size of ~100K
• micro optimize: unswitch loops, javascript does not do this for us - cost: more code
• bugfix in test/setup.js - support default + named import
• show warning on unnecessary decode_mappings. svelte.compile prefers to consume decoded mappings - transcoding of mappings is expensive
• return css.map.version = 3 in svelte.compile. solved by workaround. magic-string type DecodedSourceMap should have property version
• rebase to master -> https://github.com/milahu/svelte/tree/add-preprocessor-sourcemaps-rebased-to-ebbbc0d
• optimize: use for loops on large arrays - much faster than functional loops
• fix css remap, this must be done earlier in render_dom and render_ssr, so map.toUrl() etc can use the combined map - add test sourcemaps/compile-option-dev

removed

• show warning on potential low-resolution sourcemaps. these can make older mappings unreachable, so every preprocessor in the chain should return high-resolution sourcemaps. problem: this can not be reliably detected

todo

• cleanup commits. one to refactor tests + one to add feature preprocessor sourcemaps
• optimize: make code-red return decoded mappings. currently, mappings are encoded by code-red and decoded by compile/Component.ts
• optimize: make remapping return decoded mappings. currently, mappings are encoded by remapping and decoded by string_with_sourcemap.ts
• optimize "remapping" (combining of sourcemaps). the current format of decoded mappings (dense arrays, indexed by line and segment) is not optimal for this purpose (many lookups). currently we need binarySearch for lookup by line and column. better would be a data-structure indexed by line and column. converting between line-segment format and line-column format would be overhead, we need the sourcemap-generator to directly generate the optimized line-column format. see decode to object - make lookup faster Rich-Harris/sourcemap-codec#82 for possible solutions

todo - document

• preprocessors must generate high-resolution sourcemaps, so all segments can be traced back to original source files. this condition (hires maps) can not be easily tested by svelte, we would have to tokenize source files and compare with sourcemap.mappings = probably out of scope, rather users should fix their tools
• recommend returning decoded sourcemaps in preprocessor, to avoid unnecessary encode+decode steps
• show how to do multiple bundle steps. the bundle file must always have the filename ThisComponent.svelte. limitation: included files must be 'original source files' = leaf nodes = can not have sourcemaps
• on next release, update section svelte.preprocess: "In current versions of Svelte it will be ignored, but future versions of Svelte may take account of preprocessor sourcemaps."

bugfix in fn merge_tables

map.sources[1] was returning undefined
this was a bug in fn merge_tables, fixed in b7d5974

 function merge_tables<T>(original: T[], extended: T[]): { table: T[]; new_idx: number[] } {
   const table = original.slice();
   const new_idx = [];
-  for (let j = 0; j < original.length; j++) {
+  for (let j = 0; j < extended.length; j++) {
     const current = extended[j];
     const existing = table.indexOf(current);
     if (existing < 0) {
       table.push(current);
       new_idx[j] = table.length - 1;
     } else {
       new_idx[j] = existing;
     }
   }
   return { table, new_idx };
 }

also extended can be undefined, so we need an extra check

[benmccann]

this patch got larger than expected. let me know if i should make it smaller

It's not super obvious to me how you'd split this PR up into multiple, but if that's something that's possible I think it would help it get in faster. It is a bit intimidating to review such a large change. There are a couple files where the only edit was to add a new line to the end of the file and I think those files could be dropped from the PR

[milahu]

how you'd split this PR up into multiple

blame my strategy of "release soon and release often"

i expect the 13+ commits would be squashed into one commit, no?

some edits were revoked in the process
so for review i would just look at changes from all commits
which is "only" 1400 lines of diff

the original patch was 960 lines of diff
but i claim, my extra 440 lines are "worth it" (code readability)
i still have some refactoring todo in string_with_sourcemap.ts

There are a couple files where the only edit was to add a new line to the end of the file and I think those files could be dropped from the PR

naah, those are easy to review. git diff complains when files dont end with newline
[ we should have an eslint rule for that, as well as for indent-with-tabs ]

[benmccann]

Ignore my comments about the quotes for now. I have removed those comments. It seems the current codebase is quite inconsistent. We should set an eslint rule for that (sveltejs/eslint-config#5). Unfortunately eslint for tabs vs spaces is harder because the typescript eslint spacing rules are broken

[milahu]

concerning the sourcemap.sources issue

the sourcemap-combiner function in @ampproject/remapping
requires that all sourcemaps have only one filename in the sources field
= linear transform chain = single input + single output

in my naive understanding
remapper should support linear and convergent transform chains
with multiple input + single output files
filed an issue at ampproject/remapping#85

so the merge_tables(this.map.sources, other.map.sources) code
is technically dead code and can be removed
but we still must verify the condition of "only one source file per map"
otherwise we get a broken sourcemap

.. thats my todo for tomorrow, other than that im happy with the code

edit: done by not doing. instead, optimize fn concat:
only update mappings if sources or names has changed

concerning the eslint warnings in test/sourcemaps/samples/sourcemap-sources/_config.js

edit: this is a bug in svelte/test/setup.js

svelte/test/setup.js

Lines 7 to 13 in 46a83a5

	require.extensions['.js'] = function(module, filename) {
	const exports = [];
	let code = fs.readFileSync(filename, 'utf-8')
	.replace(/^import \* as (\w+) from ['"]([^'"]+)['"];?/gm, 'var $1 = require("$2");')
	.replace(/^import (\w+) from ['"]([^'"]+)['"];?/gm, 'var {default: $1} = require("$2");')
	.replace(/^import {([^}]+)} from ['"](.+)['"];?/gm, 'var {$1} = require("$2");')

i wonder why this import-to-require transpile is not done with babel, like

// .mocharc.js
module.exports = {
  require: ['@babel/register']
};

// .babelrc[.json]
{
  "presets": ["@babel/preset-env"]
}

//import MagicString, { Bundle } from 'magic-string';
// TODO why does this break mocha?
// SyntaxError: Cannot use import statement outside a module

import MagicString from 'magic-string';

const bundle = new MagicString.Bundle();
// eslint warning: import/no-named-as-default-member
// `MagicString` also has a named export `Bundle`.
// Check if you meant to write `import {Bundle} from 'magic-string'` instead

bundle.addSource({
	filename: "foo",
	content: new MagicString("bar")
});

so i need both MagicString and MagicString.Bundle
but either i trigger eslint warnings or i break mocha

still havent found out how mocha can even use ES6 files
without require: ['@babel/register'] in .mocharc.js
and without "presets": ["@babel/preset-env"] in .babelrc ....

getting rid off that warning would be nice

currently 1615 lines of diff vs svelte/master .... happy reviewing : /

[milahu]

a commit a day, keeps the merge day away

halfnelson was requested for review

i can only repeat what halfnelson said in #5015

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

[benmccann]

@milahu we just updated our eslint config to standardize on single quotes. Would you be able to rebase this PR against master and then make sure the new lint setup passes against this PR?

[halfnelson]

So I got around to reviewing this awesome work by @milahu . The PR as it is large, and might be best refactored into 4 smaller ones, that build on each other.

1. The refactoring of the tests
2. The new functionality in its most minimal form
3. The optimizations around multiple encode/decode and the warnings
4. The sourceMapStats structure and the sourcemap resolution drop warnings

I have had a go at 1 & 2 to get this functionality on line.

1. Test refactor Refactor SourceMap and Preprocessor tests #5583
2. Sourcemap MVP (depends on 1) b1124af

I think the only ninja edit was a rename of sourcemap_add_tostring_tourl(map) to sourcemap_define_tostring_tourl(map)

[halfnelson]

See comment about splitting the code.

We should probably keep the string | object in the Processed definition. This will avoid having to install remapper as a dependency for users of our types.

Maybe we should just define a compatible Interface def as part of svelte?

There aren't heaps of unit tests in the code (I really should have added them as part of my initial PR) but StringWithSourcemap and merge_tables and sourcemap_add_offset probably need some at some stage.

Thanks for the great work @milahu , for picking this up and running it so far, and finding the bugs :)

[benmccann]

The test refactor in #5583 has been checked in so this PR could probably be rebased to be a bit smaller now

[milahu]

progress : ]

quick note: there are two more commits hidden in
https://github.com/milahu/svelte/commits/add-preprocessor-sourcemaps-rebased-to-ebbbc0d

[halfnelson]

Thanks, it looks like I missed those two, I'll ensure they are included.

[halfnelson]

Second PR here #5584 which includes the actual source map support (without the encode/decode optimisation)