Support preexisisting inline source maps. #364

LucasSloan · 2017-02-09T02:23:15Z

If tsickle is passed sources that include inline source maps (eg ts 'sources' produced by ngc), compose those source maps with the source maps produced by running tsickle.

mprobst · 2017-02-09T16:02:51Z

src/tsickle_compiler_host.ts


-    return this.getCanonicalFileName(path.resolve(fileDir, sourceFileName));
+      sourceFile.text = sm.removeInlineSourceMap(sourceFile.text);


It's not generally safe to mutate the TypeScript AST. We've run into very hard to track bugs because of it before. Can you extract the source map on load?

Moved into the initial load.

mprobst · 2017-02-09T16:03:59Z

src/tsickle_compiler_host.ts

@@ -125,6 +126,7 @@ export class TsickleCompilerHost implements ts.CompilerHost {
    }

    const sourceFile = this.runConfiguration.oldProgram.getSourceFile(fileName);
+    this.stripAndStoreExistingSourceMap(sourceFile);


does this need to happen on every getSourceFile? Shouldn't we only do this when loading the file from disk the very first time?

Also: how often does getSourceFile get called? We shove megabytes through this, running a regexp over and over again on all inputs could get quite costly.

Moved into the first load.

mprobst · 2017-02-09T16:06:05Z

src/source_map_utils.ts

+  return getInlineSourceMapCount(source) > 0;
+}
+
+export function getInlineSourceMapCount(source: string): number {


this is only used in tests, is it really important to expose this and test it separately?

It isn't part of the public api and I'd like to keep all the source map functions together.

mprobst · 2017-02-09T16:08:25Z

src/source_map_utils.ts

+}
+
+export function extractInlineSourceMap(source: string): string {
+  const result = INLINE_SOURCE_MAP_REGEX.exec(source)!;


we've hard issues before where source code would contain:

// #sourceMapURL... more().code(); // #sourceMap...

I.e., you should take the last source map, not the first.

I might be missing something, but this still returns the first matching //# sourceMap, and the clobbering code below still replaces the first one, where it should be the last one, doesn't it?

You were right! I was reusing the same regex object, which moves the pointer in the string forward each time, rendering my test that this was happening invalid.

mprobst · 2017-02-09T16:10:12Z

src/source_map_utils.ts

+  const match = source.match(INLINE_SOURCE_MAP_REGEX);
+  if (match) {
+    // The INLINE_SOURCE_MAP_REGEX has a capture group, but we only want to count real matches
+    return match.length / 2;


Are you sure this really counts the matches?

> m = 'a a a a a a a'.match(/(a)/m).length 2

Maybe you're missing a g?

I was missing a g, thanks!

mprobst · 2017-02-09T16:14:04Z

test/e2e_source_map_test.ts

+        }`);
+
+    const closurizeSources = new Map<string, string>();
+    {


drop the extra block?

mprobst · 2017-02-09T16:41:45Z

src/source_map_utils.ts

+        `//# sourceMappingURL=data:application/json;base64,${encodedSourceMap}`);
+  } else {
+    return `${source}
+//# sourceMappingURL=data:application/json;base64,${encodedSourceMap}`;


I think this would be clearer to read if you just used \n in the string.

That is better, yes.

evmar · 2017-02-13T17:31:01Z

src/tsickle_compiler_host.ts

 import * as ts from 'typescript';

 import {convertDecorators} from './decorator-annotator';
 import {processES5} from './es5processor';
 import {ModulesManifest} from './modules_manifest';
+import * as sm from './source_map_utils';


The Go rule around short names is that the more local and contextual the name (e.g. the iterator for a "for" loop), the more likely you are to know the name in context (e.g. "i"). For a global name like this that appears across the file, a longer name like sourceMapUtils is more appropriate.

(The JS/TS style rule is to just always use long names.)

Fair enough, but why is typescript imported as ts?

Hah, that's a good point! I blame @mprobst

evmar · 2017-02-13T17:33:16Z

src/tsickle_compiler_host.ts

@@ -96,13 +96,16 @@ export class TsickleCompilerHost implements ts.CompilerHost {
  /** externs.js files produced by tsickle, if any. */
  public externs: {[fileName: string]: string} = {};

+  private sourceFileToPreexistingSourceMapMap = new Map<ts.SourceFile, SourceMapGenerator>();


You can drop one "map" suffix here, because it's implied in the type. (E.g. the other maps below don't have Map at the end.)

evmar · 2017-02-13T17:36:25Z

test/source_map_utils_test.ts

+
+import {expect} from 'chai';
+
+import * as sm from '../src/source_map_utils';


same comment here about module name, but the short name is okay-er here because it's a short file.

evmar · 2017-02-13T17:37:34Z

src/source_map_utils.ts

+
+export function sourceMapTextToGenerator(sourceMapText: string): SourceMapGenerator {
+  const sourceMapJson: any = sourceMapText;
+  return SourceMapGenerator.fromSourceMap(this.sourceMapTextToConsumer(sourceMapJson));


What's the idea behind this function and the one above it? They seem to just wrap another function?

I think the use of this here accidentally compiles but it shouldn't.

They wrap another function (and do the cast to any). When they were inlined, it was getting hard to read. And the this is intentional - we're calling another function on the same object.

Which object? I think this function is a top-level function, not a method (?)

Wow, my bad. I thought this was still where I had originally written it, despite you calling attention to it. Fixed.

mprobst · 2017-02-13T15:17:51Z

src/source_map_utils.ts

+
+export function getInlineSourceMapCount(source: string): number {
+  const match = source.match(INLINE_SOURCE_MAP_REGEX);
+  if (match) {


nit: I think it'd be more idiomatic to write return match ? match.length : 0;

mprobst · 2017-02-13T15:19:11Z

src/source_map_utils.ts

+  return source;
+}
+
+export function setInlineSourceMap(source: string, sourceMap: string): string {


can you add some docs and mention that this clobbers pre-existing maps?

mprobst · 2017-02-13T15:21:09Z

src/source_map_utils.ts

+}
+
+export function extractInlineSourceMap(source: string): string {
+  const result = INLINE_SOURCE_MAP_REGEX.exec(source)!;


I might be missing something, but this still returns the first matching //# sourceMap, and the clobbering code below still replaces the first one, where it should be the last one, doesn't it?

mprobst · 2017-02-13T19:29:11Z

I'd claim that "ts" is an obvious abbreviation in this context, whereas "sm" is not?

mprobst · 2017-02-13T22:47:54Z

src/source_map_utils.ts

@@ -0,0 +1,77 @@
+import {SourceMapConsumer, SourceMapGenerator} from 'source-map';
+
+function getInlineSourceMapRegex(): RegExp {


document why this is not using a constant.

mprobst · 2017-02-13T22:48:32Z

src/source_map_utils.ts

+  const inlineSourceMapRegex = getInlineSourceMapRegex();
+  let previousResult: RegExpExecArray|null = null;
+  let result: RegExpExecArray|null = null;
+  do {


comment why this is looping

mprobst · 2017-02-13T22:49:26Z

src/source_map_utils.ts

+}
+
+export function removeInlineSourceMap(source: string): string {
+  if (containsInlineSourceMap(source)) {


can you double check if you need the if() here? and below

Not needed here, needed below.

If tsickle is passed sources that include inline source maps (eg ts 'sources' produced by ngc), compose those source maps with the source maps produced by running tsickle.

googlebot added the cla: yes label Feb 9, 2017

LucasSloan requested review from evmar, mprobst and alexeagle February 9, 2017 02:23

mprobst reviewed Feb 9, 2017

View reviewed changes

LucasSloan force-pushed the preexisting-inline-source-maps branch from e1db360 to ea90d59 Compare February 11, 2017 02:53

evmar reviewed Feb 13, 2017

View reviewed changes

mprobst reviewed Feb 13, 2017

View reviewed changes

LucasSloan force-pushed the preexisting-inline-source-maps branch from ea90d59 to 08d31e3 Compare February 13, 2017 21:05

mprobst approved these changes Feb 13, 2017

View reviewed changes

Support preexisisting inline source maps.

c5460b2

If tsickle is passed sources that include inline source maps (eg ts 'sources' produced by ngc), compose those source maps with the source maps produced by running tsickle.

LucasSloan force-pushed the preexisting-inline-source-maps branch from 08d31e3 to c5460b2 Compare February 13, 2017 23:15

LucasSloan merged commit 7bc348c into master Feb 13, 2017

LucasSloan mentioned this pull request Feb 14, 2017

Support sourcemaps in TypeScript sources #345

Closed

mprobst deleted the preexisting-inline-source-maps branch March 16, 2017 12:33


		return this.getCanonicalFileName(path.resolve(fileDir, sourceFileName));
		sourceFile.text = sm.removeInlineSourceMap(sourceFile.text);


		import {expect} from 'chai';

		import * as sm from '../src/source_map_utils';

		@@ -0,0 +1,77 @@
		import {SourceMapConsumer, SourceMapGenerator} from 'source-map';

		function getInlineSourceMapRegex(): RegExp {

Support preexisisting inline source maps. #364

Support preexisisting inline source maps. #364

Conversation

LucasSloan commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprobst commented Feb 13, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment