diff --git a/src/googmodule.ts b/src/googmodule.ts index bf6e6d91b..56c239a02 100644 --- a/src/googmodule.ts +++ b/src/googmodule.ts @@ -29,11 +29,9 @@ export interface GoogModuleProcessorHost { * Takes the import URL of an ES6 import and returns the googmodule module * name for the imported module, iff the module is an original closure * JavaScript file. - * - * Warning: If this function is present, GoogModule won't produce diagnostics - * for multiple provides. */ - jsPathToModuleName?(importPath: string): string|undefined; + jsPathToModuleName? + (importPath: string): {name: string, multipleProvides: boolean}|undefined; /** * Takes the import URL of an ES6 import and returns the property name that * should be stripped from the usage. @@ -89,7 +87,8 @@ export function jsPathToNamespace( host: GoogModuleProcessorHost, context: ts.Node, diagnostics: ts.Diagnostic[], importPath: string, getModuleSymbol: () => ts.Symbol | undefined): string|undefined { - const namespace = localJsPathToNamespace(host, importPath); + const namespace = + localJsPathToNamespace(host, context, diagnostics, importPath); if (namespace) return namespace; const moduleSymbol = getModuleSymbol(); @@ -105,7 +104,8 @@ export function jsPathToNamespace( * Forwards to `jsPathToModuleName` on the host if present. */ export function localJsPathToNamespace( - host: GoogModuleProcessorHost, importPath: string): string|undefined { + host: GoogModuleProcessorHost, context: ts.Node|undefined, + diagnostics: ts.Diagnostic[], importPath: string): string|undefined { if (importPath.match(/^goog:/)) { // This is a namespace import, of the form "goog:foo.bar". // Fix it to just "foo.bar". @@ -113,7 +113,12 @@ export function localJsPathToNamespace( } if (host.jsPathToModuleName) { - return host.jsPathToModuleName(importPath); + const module = host.jsPathToModuleName(importPath); + if (!module) return undefined; + if (module.multipleProvides) { + reportMultipleProvidesError(context, diagnostics, importPath); + } + return module.name; } return undefined; @@ -394,10 +399,7 @@ function getGoogNamespaceFromClutzComments( findLocalInDeclarations(moduleSymbol, '__clutz_multiple_provides'); if (hasMultipleProvides) { // Report an error... - reportDiagnostic( - tsickleDiagnostics, context, - `referenced JavaScript module ${ - tsImport} provides multiple namespaces and cannot be imported by path.`); + reportMultipleProvidesError(context, tsickleDiagnostics, tsImport); // ... but continue producing an emit that effectively references the first // provided symbol (to continue finding any additional errors). } @@ -411,6 +413,15 @@ function getGoogNamespaceFromClutzComments( return actualNamespace; } +function reportMultipleProvidesError( + context: ts.Node|undefined, diagnostics: ts.Diagnostic[], + importPath: string) { + reportDiagnostic( + diagnostics, context, + `referenced JavaScript module ${ + importPath} provides multiple namespaces and cannot be imported by path.`); +} + /** * Converts a TS/ES module './import/path' into a goog.module compatible * namespace, handling regular imports and `goog:` namespace imports. diff --git a/src/summary.ts b/src/summary.ts index ffbff10fb..9f2623345 100644 --- a/src/summary.ts +++ b/src/summary.ts @@ -54,6 +54,7 @@ export class FileSummary { modName: string|undefined; autochunk = false; enhanceable = false; + legacyNamespace = false; moduleType = ModuleType.UNKNOWN; private stringify(symbol: Symbol): string { diff --git a/src/transformer_util.ts b/src/transformer_util.ts index 6c60ef0dd..4722b81d9 100644 --- a/src/transformer_util.ts +++ b/src/transformer_util.ts @@ -227,28 +227,30 @@ export function reportDebugWarning( * @param textRange pass to overrride the text range from the node with a more specific range. */ export function reportDiagnostic( - diagnostics: ts.Diagnostic[], node: ts.Node, messageText: string, textRange?: ts.TextRange, - category = ts.DiagnosticCategory.Error) { + diagnostics: ts.Diagnostic[], node: ts.Node|undefined, messageText: string, + textRange?: ts.TextRange, category = ts.DiagnosticCategory.Error) { diagnostics.push(createDiagnostic(node, messageText, textRange, category)); } function createDiagnostic( - node: ts.Node, messageText: string, textRange: ts.TextRange|undefined, + node: ts.Node|undefined, messageText: string, + textRange: ts.TextRange|undefined, category: ts.DiagnosticCategory): ts.Diagnostic { - let start, length: number; + let start: number|undefined; + let length: number|undefined; // getStart on a synthesized node can crash (due to not finding an associated // source file). Make sure to use the original node. node = ts.getOriginalNode(node); if (textRange) { start = textRange.pos; length = textRange.end - textRange.pos; - } else { + } else if (node) { // Only use getStart if node has a valid pos, as it might be synthesized. start = node.pos >= 0 ? node.getStart() : 0; length = node.end - node.pos; } return { - file: node.getSourceFile(), + file: node?.getSourceFile(), start, length, messageText, @@ -431,4 +433,4 @@ export function getPreviousDeclaration( } } return null; -} \ No newline at end of file +} diff --git a/src/ts_migration_exports_shim.ts b/src/ts_migration_exports_shim.ts index ac715764b..0136ea45e 100644 --- a/src/ts_migration_exports_shim.ts +++ b/src/ts_migration_exports_shim.ts @@ -457,6 +457,9 @@ class Generator { fileSummary.addStrongRequire({type: Type.CLOSURE, name: 'goog'}); fileSummary.addStrongRequire( {type: Type.CLOSURE, name: this.srcIds.googModuleId}); + if (maybeDeclareLegacyNameCall) { + fileSummary.legacyNamespace = true; + } fileSummary.autochunk = isAutoChunk; fileSummary.moduleType = ModuleType.GOOG_MODULE; diff --git a/test/googmodule_test.ts b/test/googmodule_test.ts index a258a7bcf..4d804cbb5 100644 --- a/test/googmodule_test.ts +++ b/test/googmodule_test.ts @@ -13,6 +13,7 @@ import * as googmodule from '../src/googmodule'; import {ModulesManifest} from '../src/modules_manifest'; import * as testSupport from './test_support'; +import {outdent} from './test_support'; interface ResolvedNamespace { name: string; @@ -42,8 +43,14 @@ function processES5( transformDynamicImport: 'closure', }; if (pathToNamespaceMap) { - host.jsPathToModuleName = (importPath: string) => - pathToNamespaceMap.get(importPath)?.name; + host.jsPathToModuleName = (importPath: string) => { + const module = pathToNamespaceMap.get(importPath); + if (!module) return undefined; + return { + name: module.name, + multipleProvides: false, + }; + }; host.jsPathToStripProperty = (importPath: string) => pathToNamespaceMap.get(importPath)?.stripProperty; } @@ -71,21 +78,6 @@ function processES5( return {output, manifest, rootDir}; } -/** - * Remove the first line (if empty) and unindents the all other lines by the - * amount of leading whitespace in the second line. - */ -function outdent(str: string) { - const lines = str.split('\n'); - if (lines.length < 2) return str; - if (lines.shift() !== '') return str; - const indent = lines[0].match(/^ */)![0].length; - for (let i = 0; i < lines.length; i++) { - lines[i] = lines[i].substring(indent); - } - return lines.join('\n'); -} - describe('convertCommonJsToGoogModule', () => { beforeEach(() => { testSupport.addDiffMatchers(); diff --git a/test/test_support.ts b/test/test_support.ts index c270ac144..6f0f2b826 100644 --- a/test/test_support.ts +++ b/test/test_support.ts @@ -453,3 +453,18 @@ export function pathToModuleName( if (fileName === tslibPath()) return 'tslib'; return cliSupport.pathToModuleName(rootModulePath, context, fileName); } + +/** + * Remove the first line (if empty) and unindents the all other lines by the + * amount of leading whitespace in the second line. + */ +export function outdent(str: string) { + const lines = str.split('\n'); + if (lines.length < 2) return str; + if (lines.shift() !== '') return str; + const indent = lines[0].match(/^ */)![0].length; + for (let i = 0; i < lines.length; i++) { + lines[i] = lines[i].substring(indent); + } + return lines.join('\n'); +} diff --git a/test/tsickle_test.ts b/test/tsickle_test.ts index 7fbb9592a..11d59e3e7 100644 --- a/test/tsickle_test.ts +++ b/test/tsickle_test.ts @@ -13,14 +13,14 @@ import {assertAbsolute} from '../src/cli_support'; import * as tsickle from '../src/tsickle'; import * as testSupport from './test_support'; +import {outdent} from './test_support'; describe('emitWithTsickle', () => { function emitWithTsickle( tsSources: {[fileName: string]: string}, tsConfigOverride: Partial = {}, tsickleHostOverride: Partial = {}, - customTransformers?: tsickle.EmitTransformers): - {[fileName: string]: string} { + customTransformers?: tsickle.EmitTransformers) { const tsCompilerOptions: ts.CompilerOptions = { ...testSupport.compilerOptions, target: ts.ScriptTarget.ES5, @@ -55,13 +55,13 @@ describe('emitWithTsickle', () => { return importPath.replace(/\/|\\/g, '.'); }, fileNameToModuleId: (fileName) => fileName.replace(/^\.\//, ''), - ...tsickleHostOverride, options: tsCompilerOptions, rootDirsRelative: testSupport.relativeToTsickleRoot, - transformDynamicImport: 'closure' + transformDynamicImport: 'closure', + ...tsickleHostOverride, }; const jsSources: {[fileName: string]: string} = {}; - tsickle.emit( + const {diagnostics} = tsickle.emit( program, tsickleHost, (fileName: string, data: string) => { jsSources[path.relative(tsCompilerOptions.rootDir!, fileName)] = data; @@ -69,7 +69,7 @@ describe('emitWithTsickle', () => { /* sourceFile */ undefined, /* cancellationToken */ undefined, /* emitOnlyDtsFiles */ undefined, customTransformers); - return jsSources; + return {jsSources, diagnostics}; } @@ -91,7 +91,7 @@ describe('emitWithTsickle', () => { const tsSources = { 'a.ts': `export const x = 1;`, }; - const jsSources = emitWithTsickle( + const {jsSources} = emitWithTsickle( tsSources, undefined, { shouldSkipTsickleProcessing: () => true, }, @@ -106,12 +106,10 @@ describe('emitWithTsickle', () => { 'b.ts': `export * from './a';`, }; - const jsSources = emitWithTsickle( - tsSources, { - preserveConstEnums: true, - module: ts.ModuleKind.ES2015, - }, - {googmodule: false}); + const {jsSources} = emitWithTsickle(tsSources, { + preserveConstEnums: true, + module: ts.ModuleKind.ES2015, + }); expect(jsSources['b.js']).toContain(`export { Foo } from './a';`); }); @@ -121,16 +119,62 @@ describe('emitWithTsickle', () => { 'a.ts': `export function f() : typeof f { return f; }`, }; - const jsSources = emitWithTsickle(tsSources, { + const {jsSources} = emitWithTsickle(tsSources, { module: ts.ModuleKind.ES2015, }); - expect(jsSources['a.js']).toContain(` -/** - * @return {function(): ?} - */ -export function f() { return f; } -`); + expect(jsSources['a.js']).toContain(outdent(` + /** + * @return {function(): ?} + */ + export function f() { return f; } + `)); + }); + + it('reports multi-provides error with jsPathToModuleName impl', () => { + const tsSources = { + 'a.ts': `import {} from 'google3/multi/provide';`, + 'clutz.d.ts': `declare module 'google3/multi/provide' { export {}; }`, + }; + const {diagnostics} = + emitWithTsickle( + tsSources, /* tsConfigOverride= */ undefined, + /* tsickleHostOverride= */ { + jsPathToModuleName(importPath: string) { + if (importPath === 'google3/multi/provide') { + return { + name: 'multi.provide', + multipleProvides: true, + }; + } + return undefined; + } + }); + expect(testSupport.formatDiagnostics(diagnostics)) + .toContain( + 'referenced JavaScript module google3/multi/provide provides multiple namespaces and cannot be imported by path'); + }); + + it('allows side-effect import of multi-provides module', () => { + const tsSources = { + 'a.ts': `import 'google3/multi/provide';`, + 'clutz.d.ts': `declare module 'google3/multi/provide' { export {}; }`, + }; + const {jsSources} = emitWithTsickle( + tsSources, /* tsConfigOverride= */ undefined, + /* tsickleHostOverride= */ { + googmodule: true, + jsPathToModuleName(importPath: string) { + if (importPath === 'google3/multi/provide') { + return { + name: 'multi.provide', + multipleProvides: true, + }; + } + return undefined; + }, + }); + expect(jsSources['a.js']).toContain(`goog.require('multi.provide');`); }); describe('regressions', () => { @@ -140,16 +184,15 @@ export function f() { return f; } 'a.ts': `export const x = 1;`, 'b.ts': `export * from './a';\n`, }; - const jsSources = emitWithTsickle( - tsSources, { - declaration: true, - module: ts.ModuleKind.ES2015, - }, - {googmodule: false}); - - expect(jsSources['b.d.ts']) - .toEqual(`//!! generated by tsickle from b.ts -export * from './a';\n`); + const {jsSources} = emitWithTsickle(tsSources, { + declaration: true, + module: ts.ModuleKind.ES2015, + }); + + expect(jsSources['b.d.ts']).toEqual(outdent(` + //!! generated by tsickle from b.ts + export * from './a'; + `)); }); }); });