From ad0fb9c6bb28730c83321537a72c10b94fb1b873 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Thu, 14 Jan 2021 11:55:57 +0000 Subject: [PATCH] fix(ngcc): copy (and update) source-maps for unmodified source files (#40429) When using the `NewEntryPointWriter` we copy unmodified files over to the new entry-point in addition to writing out the source files that are processed by ngcc. But we were not copying over associated source-map files for these unmodified source files, leading to warnings in downstream tooling. Now we will also copy over source-maps that reside as siblings of unmodified source files. We have to make sure that the sources of the source-map point to the correct files, so we also update the `sourceRoot` property of the copied source-map. Fixes #40358 PR Close #40429 --- .../writing/new_entry_point_file_writer.ts | 41 ++++++++- .../new_entry_point_file_writer_spec.ts | 85 +++++++++++++++++++ 2 files changed, 122 insertions(+), 4 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts index 47f0dff96b..15e2d8f414 100644 --- a/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts +++ b/packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts @@ -69,16 +69,49 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter { protected copyBundle( bundle: EntryPointBundle, packagePath: AbsoluteFsPath, ngccFolder: AbsoluteFsPath) { bundle.src.program.getSourceFiles().forEach(sourceFile => { - const relativePath = this.fs.relative(packagePath, absoluteFromSourceFile(sourceFile)); + const originalPath = absoluteFromSourceFile(sourceFile); + const relativePath = this.fs.relative(packagePath, originalPath); const isInsidePackage = isLocalRelativePath(relativePath); if (!sourceFile.isDeclarationFile && isInsidePackage) { - const newFilePath = this.fs.resolve(ngccFolder, relativePath); - this.fs.ensureDir(this.fs.dirname(newFilePath)); - this.fs.copyFile(absoluteFromSourceFile(sourceFile), newFilePath); + const newPath = this.fs.resolve(ngccFolder, relativePath); + this.fs.ensureDir(this.fs.dirname(newPath)); + this.fs.copyFile(originalPath, newPath); + this.copyAndUpdateSourceMap(originalPath, newPath); } }); } + /** + * If a source file has an associated source-map, then copy this, while updating its sourceRoot + * accordingly. + * + * For now don't try to parse the source for inline source-maps or external source-map links, + * since that is more complex and will slow ngcc down. + * Instead just check for a source-map file residing next to the source file, which is by far + * the most common case. + * + * @param originalSrcPath absolute path to the original source file being copied. + * @param newSrcPath absolute path to where the source will be written. + */ + protected copyAndUpdateSourceMap(originalSrcPath: AbsoluteFsPath, newSrcPath: AbsoluteFsPath): + void { + const sourceMapPath = (originalSrcPath + '.map') as AbsoluteFsPath; + if (this.fs.exists(sourceMapPath)) { + try { + const sourceMap = JSON.parse(this.fs.readFile(sourceMapPath)); + const newSourceMapPath = (newSrcPath + '.map') as AbsoluteFsPath; + const relativePath = + this.fs.relative(this.fs.dirname(newSourceMapPath), this.fs.dirname(sourceMapPath)); + sourceMap['sourceRoot'] = this.fs.join(relativePath, sourceMap['sourceRoot'] || '.'); + this.fs.ensureDir(this.fs.dirname(newSourceMapPath)); + this.fs.writeFile(newSourceMapPath, JSON.stringify(sourceMap)); + } catch (e) { + this.logger.warn(`Failed to process source-map at ${sourceMapPath}`); + this.logger.warn(e.message ?? e); + } + } + } + protected writeFile(file: FileToWrite, packagePath: AbsoluteFsPath, ngccFolder: AbsoluteFsPath): void { if (isDtsPath(file.path.replace(/\.map$/, ''))) { diff --git a/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts b/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts index 83d03feb9f..92406496c8 100644 --- a/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts +++ b/packages/compiler-cli/ngcc/test/writing/new_entry_point_file_writer_spec.ts @@ -51,6 +51,12 @@ runInEachFileSystem(() => { {name: _('/node_modules/test/esm5.js'), contents: 'export function FooTop() {}'}, {name: _('/node_modules/test/esm5.js.map'), contents: 'ORIGINAL MAPPING DATA'}, {name: _('/node_modules/test/es2015/index.js'), contents: 'export {FooTop} from "./foo";'}, + { + name: _('/node_modules/test/es2015/index.js.map'), + contents: + '{"version":3,"file":"index.js","sources":["../src/index.ts"],"mappings":"AAAA"}' + }, + {name: _('/node_modules/test/src/index.ts'), contents: 'export {FooTop} from "./foo";'}, {name: _('/node_modules/test/es2015/foo.js'), contents: 'export class FooTop {}'}, { name: _('/node_modules/test/a/package.json'), @@ -154,6 +160,85 @@ runInEachFileSystem(() => { .toEqual('export {FooTop} from "./foo";'); }); + it('should copy any source-map for unmodified files in the program (adding missing sourceRoot)', + () => { + // Ensure source-mapping for a non-processed source file `index.js`. + const sourceMap = { + version: 3, + file: 'index.js', + sources: ['../src/index.ts'], + mappings: 'AAAA', + }; + loadTestFiles([ + { + name: _('/node_modules/test/es2015/index.js.map'), + contents: JSON.stringify(sourceMap) + }, + {name: _('/node_modules/test/src/index.ts'), contents: 'export {FooTop} from "./foo";'} + ]); + + // Simulate that only the `foo.js` file was modified + const modifiedFiles = [{ + path: _('/node_modules/test/es2015/foo.js'), + contents: 'export class FooTop {} // MODIFIED' + }]; + fileWriter.writeBundle(esm2015bundle, modifiedFiles, ['es2015']); + + expect(JSON.parse(fs.readFile(_('/node_modules/test/__ivy_ngcc__/es2015/index.js.map')))) + .toEqual({...sourceMap, sourceRoot: '../../es2015'}); + }); + + it('should copy any source-map for unmodified files in the program (updating sourceRoot)', + () => { + // Ensure source-mapping for a non-processed source file `index.js`. + const sourceMap = { + version: 3, + file: 'index.js', + sourceRoot: '../src', + sources: ['index.ts'], + mappings: 'AAAA', + }; + loadTestFiles([ + { + name: _('/node_modules/test/es2015/index.js.map'), + contents: JSON.stringify(sourceMap) + }, + {name: _('/node_modules/test/src/index.ts'), contents: 'export {FooTop} from "./foo";'} + ]); + + // Simulate that only the `foo.js` file was modified + const modifiedFiles = [{ + path: _('/node_modules/test/es2015/foo.js'), + contents: 'export class FooTop {} // MODIFIED' + }]; + fileWriter.writeBundle(esm2015bundle, modifiedFiles, ['es2015']); + + expect(JSON.parse(fs.readFile(_('/node_modules/test/__ivy_ngcc__/es2015/index.js.map')))) + .toEqual({...sourceMap, sourceRoot: '../../src'}); + }); + + it('should ignore (with a warning) any invalid source-map for unmodified files in the program', + () => { + // Ensure source-mapping for a non-processed source file `index.js`. + loadTestFiles([ + {name: _('/node_modules/test/es2015/index.js.map'), contents: 'INVALID JSON STRING'}, + {name: _('/node_modules/test/src/index.ts'), contents: 'export {FooTop} from "./foo";'} + ]); + + // Simulate that only the `foo.js` file was modified + const modifiedFiles = [{ + path: _('/node_modules/test/es2015/foo.js'), + contents: 'export class FooTop {} // MODIFIED' + }]; + fileWriter.writeBundle(esm2015bundle, modifiedFiles, ['es2015']); + + expect(fs.exists(_('/node_modules/test/__ivy_ngcc__/es2015/index.js.map'))).toBe(false); + expect(logger.logs.warn).toEqual([ + [`Failed to process source-map at ${_('/node_modules/test/es2015/index.js.map')}`], + ['Unexpected token I in JSON at position 0'], + ]); + }); + it('should update the package.json properties', () => { fileWriter.writeBundle( esm5bundle,