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

add: parse attached sourcemap from preprocessor #5854

Merged
merged 7 commits into from
Jan 19, 2021

Conversation

milahu
Copy link
Contributor

@milahu milahu commented Jan 3, 2021

solve issue sveltejs/svelte-preprocess#286

allow preprocessors to return sourcemaps not only in result.map
but also attached to result.code as 'magic comment' in script or style

var code = 'here';
/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW ... vgICB9XG4iXX0= */

or in script

var code = 'here';
//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW ... vgICB9XG4iXX0=

the attached sourcemap is removed, so the browser does not double-parse the sourcemap
that was the original problem in issue 286
this removal is asserted by the test

for now, support only one sourcemap, either in result.map or attached to result.code, not both --> fatal error

also for now, this works only for script and style preprocessors
in theory, the 'magic comment' strategy is also possible in html

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

thanks! this looks pretty good to me

@dmitrage do you want to take a look at this as well before it's merged?

src/compiler/utils/string_with_sourcemap.ts Outdated Show resolved Hide resolved
test/sourcemaps/samples/attached-sourcemap/test.js Outdated Show resolved Hide resolved
test/sourcemaps/index.ts Show resolved Hide resolved
Copy link
Contributor

@dmitrage dmitrage left a comment

Choose a reason for hiding this comment

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

Nice! Quick review summary:

  • please add support for single line comments
  • ensure offsets are OK if the last line with comment had mapping (edge case)

And suggestion (for future PR?): maybe we should also remove external sourceMappingURL comment from preprocessed.code when preprocessed.map exists?

src/compiler/utils/string_with_sourcemap.ts Outdated Show resolved Hide resolved
src/compiler/utils/string_with_sourcemap.ts Outdated Show resolved Hide resolved
@milahu
Copy link
Contributor Author

milahu commented Jan 5, 2021

done regex

speculation: scss adds the magic comment without a newline, like

div {
  color: red;
}/*# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW ... vgICB9XG4iXX0= */

todo:
find attached sourcemap without seeking to last newline (in worst case, there is no newline at all)

simply use a regex? like
\/[/*]# sourceMappingURL=data:application\/json;\S*base64,([a-zA-Z0-9+/]+={,2}) ?(?:\*\/)?$

\S* matches charset=utf-8; for example

related:

with regex, still errors with scss, which returns result.map = SourceMapGenerator(....)

possible problem:
in result.map.sources, the file input.svelte is duplicated = relative path + absolute url
(slightly related to #5793)

for example

result.map.sources = [
  'file:///absolute/path/to/src/components/example.svelte',
  'src/components/example.svelte' // = the filename passed to preprocessor
]

but file_basename = 'example.svelte' in

// offset only segments pointing at original component source
const source_index = decoded_map.sources.indexOf(file_basename);
if (source_index !== -1) {
	sourcemap_add_offset(decoded_map, get_location(offset + prefix.length), source_index);
}

which should look more like

decoded_map.sources
	.map((s, i) => ([s, i]))
	.filter(([s, _i]) => get_file_basename(s as string) == file_basename)
	.forEach(([_s, source_index]) => {
		sourcemap_add_offset(decoded_map, get_location(offset + prefix.length), (source_index as number));
	})
;

.. but i still get the off-by-one error with scss *and* with css - nasty bug!

scss sets both result.map = SourceMapGenerator(....)
and result.code = '.... /*# sourceMappingURL=src/components/example.svelte.map */'
which may cause problems

@Maggi64
Copy link

Maggi64 commented Jan 14, 2021

@milahu What is the current status on this PR? Are we waiting for that Sass PR?

@milahu
Copy link
Contributor Author

milahu commented Jan 14, 2021

this PR is pretty-much finished

what could be added is: if result.map is set, remove /*# sourceMappingURL=input.ext.map */ from result.code
to avoid double-parsing of sourcemaps in the browser
ideally this is disabled in the preprocessor config, for example { omitSourceMapUrl: true } in sass

but regarding issue sveltejs/svelte-preprocess#286
there must be another bug, probably in svelte, maybe in svelte-preprocess

in general, the problem is caused by transforming code before the <script> tag
still looking for a *minimal* reproduction of that bug
probably related to markup preprocessor --> no handling of missing_lines, no parsing of attached sourcemap

	// Combine all the source maps for each preprocessor function into one
	const map: RawSourceMap = combine_sourcemaps(
		file_basename,
		sourcemap_list
	);

at this point of svelte.preprocess the mapping is correct --> blame svelte.compile?
one problem is: sourcemap support in rollup-plugin-svelte is not yet released
workaround: npm i -D https://github.com/sveltejs/rollup-plugin-svelte.git

@milahu
Copy link
Contributor Author

milahu commented Jan 18, 2021

what could be added is: if result.map is set, remove /*# sourceMappingURL=input.ext.map */ from result.code
to avoid double-parsing of sourcemaps in the browser

done. typescript does this, and there is no way to disable, so we must remove manually
tested manually, feels too trivial to add a formal test

		// sourceMappingURL is file path or URL
		if (!processed.map) {
			// TODO show warning? silently ignore?
			throw_error('value error. processed.map is empty, '+
				'but processed.code has attached sourcemap file '+map_url+'. '+
				'please make your preprocessor return a sourcemap.');
		}

currently thats a fatal error, maybe can be ignored

@benmccann
Copy link
Member

Thanks for this! Were there any other changes you wanted to get in after updating the warning message or will it be ready to merge after that?

@milahu
Copy link
Contributor Author

milahu commented Jan 19, 2021

ready : )

@benmccann benmccann merged commit 0d19f67 into sveltejs:master Jan 19, 2021
@Conduitry
Copy link
Member

This has been released in 3.32.0 - thank you!

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.

7 participants