Skip to content

Do not emit names and name index mapping in source map #5713

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

Merged
merged 4 commits into from
Dec 1, 2015

Conversation

sheetalkamat
Copy link
Member

Since source map spec isn't clear about use of names, removing names entry all together. We can look at the symbol mapping once there is update to the spec about it.

Since sourcemap spec is not very clear about symbol translation and
use of nameIndex of the mapping, dont emit it
@develar
Copy link

develar commented Nov 19, 2015

Hi, what do you mean? Spec is clear and IntelliJ Platform uses it. Please don't break debug!

  1. Find mapping entry.
  2. Get mapping entry name
  3. Name is missed (unnamed entry) - use generated element name. name is presented - use it.

Please don't break existing funcitonality and fix #5224. I will answer to any question and can provide closed source code (partially opened - see https://github.com/JetBrains/intellij-community/blob/master/platform/script-debugger/debugger-ui/src/com/jetbrains/javascript/debugger/NameMapper.kt) As you can see in the provided NameMapper, there is a flag js.debugger.name.mappings.by.source.code as a workaround of 5224.

Our users happy to use TypeScript and debug it.

@asvetliakov
Copy link

Please do not merge this at least until implementing another solution for debugging
Thanks

@sheetalkamat
Copy link
Member Author

@develar we have never emitted variable names in the source map. We were emitting the callstack name it belonged too.. So functionality you are talking about is anyways broken and unsupported without this change.

@develar
Copy link

develar commented Nov 20, 2015

@sheetalkamat So, if you don't mangle names, it is ok (nothing to map). Thanks for clarification.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 25, 2015

can you coordinate this with #5780

@mhegazy
Copy link
Contributor

mhegazy commented Nov 26, 2015

The easiest is to wait for @rbuckton to merge #5780 and then apply the change on top, the tests should be unchanged.

@rbuckton
Copy link
Member

@sheetalkamat Do you have a preference?

@sheetalkamat
Copy link
Member Author

@rbuckton, Probably you should go first as my changes are small, I think it would be easier if I have to merge.

@rbuckton
Copy link
Member

@sheetalkamat: Alright, I've gone ahead and merged my change.

sheetalkamat added a commit that referenced this pull request Dec 1, 2015
Do not emit names and name index mapping in source map
@sheetalkamat sheetalkamat merged commit dac1874 into master Dec 1, 2015
@sheetalkamat sheetalkamat deleted the noSourcemapNames branch December 1, 2015 00:45
@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Dec 1, 2015
@mhegazy mhegazy added this to the TypeScript 1.8 milestone Dec 1, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Dec 1, 2015

@develar
Copy link

develar commented Mar 26, 2016

Without name mappings it is not possible to debug this in the lambdas — https://youtrack.jetbrains.com/issue/WEB-20340#comment=27-1372090

_this is generated but TS sourcemap doesn't contain name mappings and, obviously, IDEA cannot remap it to real this...

screen shot 2016-03-26 at 15 50 03

{"version":3,"file":"myclass1_spec.js","sourceRoot":"","sources":["../../src/__tests__/myclass1_spec.ts"],"names":[],"mappings":"AAAA,IAAI,GAAG,GAAG,EAET,CAAA;AAED;IAAA;QACE,QAAG,GAAG,IAAI,CAAA;IAOZ,CAAC;IALC,oBAAG,GAAH;QAAA,iBAIC;QAHC,UAAU,CAAC;YACT,OAAO,CAAC,GAAG,CAAC,KAAI,CAAC,GAAG,CAAC,CAAA;QACvB,CAAC,EAAE,EAAE,CAAC,CAAA;IACR,CAAC;IACH,aAAC;AAAD,CAAC,AARD,IAQC;AAED,IAAI,MAAM,EAAE,CAAC,GAAG,EAAE,CAAA"}

@born2net
Copy link

+1

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants