Skip to content

Commit

Permalink
Support multi provide modules with jsPathToModuleName implementation
Browse files Browse the repository at this point in the history
PiperOrigin-RevId: 585628039
  • Loading branch information
frigus02 authored and copybara-github committed Nov 29, 2023
1 parent d5c2921 commit 819cdaf
Show file tree
Hide file tree
Showing 7 changed files with 132 additions and 65 deletions.
33 changes: 22 additions & 11 deletions src/googmodule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -105,15 +104,21 @@ 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".
return importPath.substring('goog:'.length);
}

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;
Expand Down Expand Up @@ -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).
}
Expand All @@ -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.
Expand Down
1 change: 1 addition & 0 deletions src/summary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export class FileSummary {
modName: string|undefined;
autochunk = false;
enhanceable = false;
legacyNamespace = false;
moduleType = ModuleType.UNKNOWN;

private stringify(symbol: Symbol): string {
Expand Down
16 changes: 9 additions & 7 deletions src/transformer_util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -431,4 +433,4 @@ export function getPreviousDeclaration(
}
}
return null;
}
}
3 changes: 3 additions & 0 deletions src/ts_migration_exports_shim.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
26 changes: 9 additions & 17 deletions test/googmodule_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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();
Expand Down
15 changes: 15 additions & 0 deletions test/test_support.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
103 changes: 73 additions & 30 deletions test/tsickle_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ts.CompilerOptions> = {},
tsickleHostOverride: Partial<tsickle.TsickleHost> = {},
customTransformers?: tsickle.EmitTransformers):
{[fileName: string]: string} {
customTransformers?: tsickle.EmitTransformers) {
const tsCompilerOptions: ts.CompilerOptions = {
...testSupport.compilerOptions,
target: ts.ScriptTarget.ES5,
Expand Down Expand Up @@ -55,21 +55,21 @@ 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;
},
/* sourceFile */ undefined,
/* cancellationToken */ undefined, /* emitOnlyDtsFiles */ undefined,
customTransformers);
return jsSources;
return {jsSources, diagnostics};
}


Expand All @@ -91,7 +91,7 @@ describe('emitWithTsickle', () => {
const tsSources = {
'a.ts': `export const x = 1;`,
};
const jsSources = emitWithTsickle(
const {jsSources} = emitWithTsickle(
tsSources, undefined, {
shouldSkipTsickleProcessing: () => true,
},
Expand All @@ -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';`);
});
Expand All @@ -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', () => {
Expand All @@ -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';
`));
});
});
});

0 comments on commit 819cdaf

Please sign in to comment.