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

Use inlineSources to bundle source file contents inside map files #7615

Merged
merged 9 commits into from
Mar 11, 2020

Conversation

xirzec
Copy link
Member

@xirzec xirzec commented Mar 3, 2020

Today when TSC generates sourcemaps, it doesn't include the source file contents, which means we have to include the src directory (which we didn't always do correctly.)

To sidestep this problem, we can tell the compiler to always include the source file contents, which should make it easier for consumers of the esm sources to manage using the map files.

Fixes #7544

@xirzec xirzec added the Client This issue points to a problem in the data-plane of the library. label Mar 3, 2020
@xirzec xirzec self-assigned this Mar 3, 2020
@richardpark-msft
Copy link
Member

@xirzec - any obvious negatives to this approach?

@xirzec
Copy link
Member Author

xirzec commented Mar 3, 2020

@xirzec - any obvious negatives to this approach?

The map files are bigger, but considering they are useless without the TS files, it's not like there is a way we could avoid shipping the TS file contents in some form or another anyway.

@chradek
Copy link
Contributor

chradek commented Mar 3, 2020

Would it be possible to update our SDK guidelines to also recommend this approach, vs the current approach of shipping all the TS files?

@xirzec
Copy link
Member Author

xirzec commented Mar 3, 2020

Would it be possible to update our SDK guidelines to also recommend this approach, vs the current approach of shipping all the TS files?

Yep, good idea

Copy link
Contributor

@sadasant sadasant left a comment

Choose a reason for hiding this comment

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

This is awesome! If CI is working fine, we should get this in this week. Probably after the preview release?

@xirzec
Copy link
Member Author

xirzec commented Mar 10, 2020

image
screenshot showing the dev experience - VS Code shows a source TS file from the package, even though you can see it's not part of the package on disk

@willmtemple
Copy link
Contributor

@xirzec with this change, if I use go-to-definition will we still be taken to the actual source file on disk if it is available? I.e. if I observe an issue in a test or something that is resolving to another package, and I go to its definition, will I end up on the actual source file or on a reconstructed buffer that contains the sources from the map files?

@xirzec
Copy link
Member Author

xirzec commented Mar 10, 2020

@xirzec with this change, if I use go-to-definition will we still be taken to the actual source file on disk if it is available? I.e. if I observe an issue in a test or something that is resolving to another package, and I go to its definition, will I end up on the actual source file or on a reconstructed buffer that contains the sources from the map files?

It seems to honor going to the real source files. I tested it with going to definition on a function from core-http from inside search after rebuilding with the flag.

@xirzec xirzec merged commit 0343a0d into Azure:master Mar 11, 2020
@xirzec xirzec deleted the inlineSources branch March 11, 2020 17:34
@jeremymeng
Copy link
Member

😢 go-to-def doesn't seem to work with my Emacs configuration. When I test with abort-controller package it went to the d.ts file. Maybe I will wait for @daviwil Emacs tricks to get it working

@xirzec
Copy link
Member Author

xirzec commented Mar 11, 2020

😢 go-to-def doesn't seem to work with my Emacs configuration. When I test with abort-controller package it went to the d.ts file. Maybe I will wait for @daviwil Emacs tricks to get it working

I noticed even in the old world (pre-inline sources) sometimes go to def would hit the definitions instead for certain packages/types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of compiler options instead of reshipping source files
8 participants