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

Generate source maps #223

Closed
alangpierce opened this issue May 29, 2018 · 9 comments
Closed

Generate source maps #223

alangpierce opened this issue May 29, 2018 · 9 comments

Comments

@alangpierce
Copy link
Owner

Mentioned by @Rich-Harris in #221.

Currently, Sucrase preserves line numbers, which gets many of the benefits of source maps (like reliable stack traces when using sucrase/register), but ideally it would emit real source maps. If we just care about lines, I think it would be a trivial source map that maps each line to itself. Mapping tokens within each line (which I think is useful for debuggers) should also be doable, but would need to be more integrated into the code generator.

@aleclarson
Copy link
Contributor

Have you started on this yet? This is my most desired feature. :)

@alangpierce
Copy link
Owner Author

Nope, not yet. I'm hoping to get to it this weekend. Happy to collaborate if you want to take any of it on or do some research. It'll probably require a breaking change (currently Sucrase just returns a string, and I guess it'll need to return an object with both), so that's another thing I'll need to figure out.

@aleclarson
Copy link
Contributor

I might be able to help on the import/export side of things, but I have no time (or desperate need) for TS and Flow. Also, you should create a sourcemaps branch once you get started (and add me as a contributor) so I don't need to ask you about progress. Cheers!

@alangpierce
Copy link
Owner Author

@aleclarson do you know what the best way is to test that source maps are working right? I actually don't have a great sense of how tools use source mappings within lines. Maybe it's needed for hover-to-view-value when debugging in Chrome DevTools (or WebStorm or VSCode), or if declared names get transformed, it uses the mapping to allow referencing the source code name from the console?

Also, my current plan is to have two modes (like magic-string has): one that only does line mappings and one that maps everything. Line mappings are a little pointless since the lines all correspond to the original source code, but I could see it being nice to have something in the source map format that you can feed into other tools. Do you think you'd use the full mode?

@alangpierce
Copy link
Owner Author

alangpierce commented Jun 16, 2018

Oh, and hopefully this can be done at a lower level than any particular transform and automatically work for all of them at once (similar to how magic-string doesn't require transform-specific knowledge to make the source map). I should be able to get started on it pretty soon (the 3.0 release unblocked this since now we can return more than just the code), but I'm a bit new to the details of the sourcemap format, so I'll need to do some research first.

alangpierce added a commit that referenced this issue Jun 17, 2018
Progress toward #223

This is a first pass that's meant to be simple, flexible, and fairly
unopinionated. Notably, it does NOT yet compute mappings within each line, and
instead just maps each line to the corresponding line in the source. I'll
implement smarter mappings in the future, possibly as an option if it ends up
hurting performance.

A quick benchmark shows that enabling source maps increases total running time
by about 25%, even with this simple approach. Hopefully that can be optimized in
the future.

Another possible to-do item is for Sucrase to accept a source map which it then
modifies. However, this can hopefully be handled by other tools that compose
source maps.

There are also various tweaks that may make this more convenient, depending on
use case.
* Optionally include the original source code in `sourcesContent` in the source
  map.
* Optionally append a sourceMappingURL comment to the end of the source code.
* Optionally return the source map as a string instead of an object.
* Better control over the sourceRoot and file paths specified in the source map.

All of these use cases can be satisfied with just one or two lines of code after
running Sucrase, though, so none seem super critical.

Some docs of other tools emitting source maps, which I used for inspiration:
https://babeljs.io/docs/en/babel-core
https://www.typescriptlang.org/docs/handbook/compiler-options.html
alangpierce added a commit that referenced this issue Jun 17, 2018
Progress toward #223

This is a first pass that's meant to be simple, flexible, and fairly
unopinionated. Notably, it does NOT yet compute mappings within each line, and
instead just maps each line to the corresponding line in the source. I'll
implement smarter mappings in the future, possibly as an option if it ends up
hurting performance.

A quick benchmark shows that enabling source maps increases total running time
by about 25%, even with this simple approach. Hopefully that can be optimized in
the future.

Another possible to-do item is for Sucrase to accept a source map which it then
modifies. However, this can hopefully be handled by other tools that compose
source maps.

There are also various tweaks that may make this more convenient, depending on
use case.
* Optionally include the original source code in `sourcesContent` in the source
  map.
* Optionally append a sourceMappingURL comment to the end of the source code.
* Optionally return the source map as a string instead of an object.
* Better control over the sourceRoot and file paths specified in the source map.

All of these use cases can be satisfied with just one or two lines of code after
running Sucrase, though, so none seem super critical.

Some docs of other tools emitting source maps, which I used for inspiration:
https://babeljs.io/docs/en/babel-core
https://www.typescriptlang.org/docs/handbook/compiler-options.html
alangpierce added a commit that referenced this issue Jun 17, 2018
Progress toward #223

This is a first pass that's meant to be simple, flexible, and fairly
unopinionated. Notably, it does NOT yet compute mappings within each line, and
instead just maps each line to the corresponding line in the source. I'll
implement smarter mappings in the future, possibly as an option if it ends up
hurting performance.

A quick benchmark shows that enabling source maps increases total running time
by about 25%, even with this simple approach. Hopefully that can be optimized in
the future.

Another possible to-do item is for Sucrase to accept a source map which it then
modifies. However, this can hopefully be handled by other tools that compose
source maps.

There are also various tweaks that may make this more convenient, depending on
use case.
* Optionally include the original source code in `sourcesContent` in the source
  map.
* Optionally append a sourceMappingURL comment to the end of the source code.
* Optionally return the source map as a string instead of an object.
* Better control over the sourceRoot and file paths specified in the source map.

All of these use cases can be satisfied with just one or two lines of code after
running Sucrase, though, so none seem super critical.

Some docs of other tools emitting source maps, which I used for inspiration:
https://babeljs.io/docs/en/babel-core
https://www.typescriptlang.org/docs/handbook/compiler-options.html
@aleclarson
Copy link
Contributor

do you know what the best way is to test that source maps are working right?

I'm not aware of any automated source map testing tools. 😢 I think unit-testing the decoded mappings and manual debugger testing are the only way (but I could be mistaken).

I actually don't have a great sense of how tools use source mappings within lines. Maybe it's needed for hover-to-view-value when debugging in Chrome DevTools (or WebStorm or VSCode), or if declared names get transformed, it uses the mapping to allow referencing the source code name from the console?

Column mappings let you set breakpoints in the original source (especially useful when lines and columns are shifted, or multiple expressions exist on the same line). I don't think Chrome lets you use the source names in the console, but maybe others do. I'm not entirely sure how debuggers use the names array, but it's best to populate it with renamed identifiers.

Also, my current plan is to have two modes (like magic-string has): one that only does line mappings and one that maps everything.

I'm pretty sure magic-string always does column mappings. It's not avoidable because of the library's flexibility. The hires option makes the source map even heavier, but I don't know in which scenarios it does that (or why I would ever need that).

I should be able to get started on it pretty soon (the 3.0 release unblocked this since now we can return more than just the code), but I'm a bit new to the details of the sourcemap format, so I'll need to do some research first.

Good to hear! Are you going to implement most of it into the TokenProcessor? I haven't studied the transformations that Sucrase makes, but I bet a lot of it will be straight-forward. Often enough, replacement (replacing something with something), insertion (replacing nothing with something), and removal (replacing something with nothing) all require two mappings (one for the start column being replaced, and one for the end column).

@alangpierce
Copy link
Owner Author

I'm pretty sure magic-string always does column mappings

Yep, you're right. Looks like hires maps each individual character within unchanged regions, unclear when that's useful.

Column mappings let you set breakpoints in the original source

I guess I'm still having trouble finding an example where column mappings improve the developer experience with Sucrase in particular. I guess one thing I could do is map class field initializers to the field line, like in this playground link, but even that seems a bit obscure.

Are you going to implement most of it into the TokenProcessor?

Yeah, that was what I had in mind, although I'm also happy so far with the line-only mappings. My main worry with column mappings is that it would significantly hurt performance. A quick prototype shows it as a 2x slowdown. Even adding conditional checks could make things slower with source maps disabled, but I guess I'd need to test the effect of that, and probably it would be a well-predicted branch. TokenProcessor also has a snapshot/restore thing that doesn't work so well with source maps being built.

I guess it's a weird balance. I want Sucrase to be as fast as possible, but obviously the underlying reason is so it can be a great developer tool, so a compromise that hurts the developer experience doesn't makes sense. I guess I'm hoping there's a solution that has near zero impact on perf and also gets all of the practical benefits to developers. For example, if the names array is never actually used by tools, I'd rather not hurt performance populating it.

To-do items:

  • Add an option for the compiled filename to be included in the source map.
  • Implement improve source maps with column mappings in TokenProcessor, probably behind a setting.
  • See if I can improve performance in generating the existing "identity source map".

Open questions:

  • Do we include or exclude sourcesContent by default? I'll leave it out for now, but adding it by default wouldn't be a breaking change (in my opinion).
  • Do we add an option to append a sourceMappingURL directly? As one argument against, ts-node needs to do some hacks to modify the source map and swap out the incorrect generated one with the fixed one. But adding the option shouldn't hurt when it does work.

alangpierce added a commit that referenced this issue Jun 18, 2018
Progress toward #223.

This should make it more easily extensible in the future. We also now accept a
`compiledFilename` option for the filename to go in the source map.
alangpierce added a commit that referenced this issue Jun 18, 2018
Progress toward #223.

This should make it more easily extensible in the future. We also now accept a
`compiledFilename` option for the filename to go in the source map.
@alangpierce
Copy link
Owner Author

Closing for now since I've been using source maps for a while now for WebStorm debugging and have been happy with it. The line-only approach now works in all cases (I think) now that #313 has been implemented, and I like the simplicity of not having to worry about column mappings.

If source maps are lacking for any concrete use cases, I'd be happy to take a look in a follow-up issue, but I'll close this one since the core use case seems to be met.

@ersinakinci
Copy link

@alangpierce, I'm curious why there isn't an option to generate source maps in the CLI tool? I'd imagine that's what's needed for 99% of cases. Otherwise, most people probably wouldn't be aware that Sucrase can generate source maps (as a number of open issues attest to).

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

No branches or pull requests

3 participants