Fix #281 Enabling debugging for TS when compiled into a single JS file #266
Conversation
|
Hi @ahmad-farid, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
| if (originalFileName == null) { | ||
| module = new NodeModule(module.Id, javaScriptFileName); | ||
| } else { | ||
| } else { |
There was a problem hiding this comment.
Looks like you added a bunch of trailing whitespace here. Please remove.
| originalFileName = SourceMapper.MapToOriginal(javaScriptFileName); | ||
| } | ||
|
|
||
| return originalFileName; |
There was a problem hiding this comment.
This logic would be more readable with a null-coalescing operator
return originalFileName ?? SourceMapper.MapToOriginal(javaScriptFileName);
| /// </summary> | ||
| /// <param name="javaScriptFileName">JavaScript compiled file.</param> | ||
| /// <param name="stackFrame">The current stack frame.</param> | ||
| internal string GetOriginalFileName(string javaScriptFileName, NodeStackFrame stackFrame) { |
There was a problem hiding this comment.
Not a fan of conflating the debugging logic with the source mapping logic... probably best to move the core logic to SourceMapper.cs (which would accepts line/column arguments), and use a helper here if necessary. This would also simplify the unit test.
|
@gilbertw - you have more experience with this part of the codebase - mind chiming in? |
|
left one more comment you may not have seen (girhub notifications are annoying), but lgtm other than that |
| @@ -21,12 +21,13 @@ | |||
| using System.Windows.Threading; | |||
| using System.Xml.Linq; | |||
| using Microsoft.NodejsTools; | |||
| using Microsoft.NodejsTools.Debugger; | |||
There was a problem hiding this comment.
you shouldn't need this reference anymore, right?
mousetraps
changed the title
Fix #281 Enabling debugging for TS when compiled into a single JS file
#281