Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/preprocessor sourcemaps #5428

Closed
wants to merge 26 commits into from

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Sep 19, 2020

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

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

halfnelson and others added 12 commits September 18, 2020 20:32
sort devDependencies

rename to compile_options.sourcemap

file src/compiler/utils/string_with_map.ts
* indent with tabs
* run prettier
* remove extra whitespace
* add newline at end of file
* wrap long lines
* rephrase some comments
move string_with_map.ts to string_with_sourcemap.ts
rename GeneratedStringWithMap to StringWithSourcemap
rename offset_source_location to sourcemap_add_offset
rename source_maps to sourcemap_list
rename source_locator to get_location
rename as_sourcemap to get_sourcemap
rename encode to sourcemap_encode
move { brace to line before
add some comments
rename decode to sourcemap_decode

allow self-closing <style/>
move fn replace_async back to old place to reduce line diffs
fn preprocess: allow empty argument `preprocessor` (noop)
make fns replace_async, get_replacement pure fns
  pass filename, get_location as argument
restore old format to reduce line diffs
remove empty fields of sourcemap: file, sourcesContent
use _config.js, indent with tabs, rename variables,
@milahu milahu closed this Sep 20, 2020
@milahu milahu reopened this Sep 20, 2020
@benmccann
Copy link
Member

benmccann commented Sep 21, 2020

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
Copy link
Contributor Author

milahu commented Sep 21, 2020

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
Copy link
Member

benmccann commented Sep 21, 2020

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
Copy link
Contributor Author

milahu commented Sep 23, 2020

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 milahu force-pushed the feature/preprocessor-sourcemaps branch from 3fe1fcc to 422cc0d Compare September 26, 2020 07:51
@milahu
Copy link
Contributor Author

milahu commented Sep 29, 2020

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
Copy link
Member

@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?

test/sourcemaps/index.ts Outdated Show resolved Hide resolved
@halfnelson
Copy link
Contributor

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)

Copy link
Contributor

@halfnelson halfnelson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@milahu
Copy link
Contributor Author

milahu commented Oct 25, 2020

progress : ]

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

@halfnelson
Copy link
Contributor

halfnelson commented Oct 25, 2020 via email

@halfnelson
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants