Skip to content
Permalink
Browse files

fix(ngcc): always add exports for `ModuleWithProviders` references (#…

…33875)

In #32902 a bug was supposedly fixed where internal classes as used
within `ModuleWithProviders` are publicly exported, even when the
typings file already contained the generic type on the
`ModuleWithProviders`. This fix turns out to have been incomplete, as
the `ModuleWithProviders` analysis is not done when not processing the
typings files.

The effect of this bug is that formats that are processed after the
initial format had been processed would not have exports for internal
symbols, resulting in "export '...' was not found in '...'" errors.

This commit fixes the bug by always running the `ModuleWithProviders`
analyzer. An integration test has been added that would fail prior to
this change.

Fixes #33701

PR Close #33875
  • Loading branch information
JoostK authored and alxhub committed Nov 16, 2019
1 parent a05e42a commit f3d8f6adca40f0b702d3daf997426304570cd80a
@@ -30,7 +30,9 @@ export type ModuleWithProvidersAnalyses = Map<ts.SourceFile, ModuleWithProviders
export const ModuleWithProvidersAnalyses = Map;

export class ModuleWithProvidersAnalyzer {
constructor(private host: NgccReflectionHost, private referencesRegistry: ReferencesRegistry) {}
constructor(
private host: NgccReflectionHost, private referencesRegistry: ReferencesRegistry,
private processDts: boolean) {}

analyzeProgram(program: ts.Program): ModuleWithProvidersAnalyses {
const analyses = new ModuleWithProvidersAnalyses();
@@ -43,16 +45,19 @@ export class ModuleWithProvidersAnalyzer {
this.referencesRegistry.add(fn.ngModule.node, new Reference(fn.ngModule.node));
}

const dtsFn = this.getDtsDeclarationForFunction(fn);
const typeParam = dtsFn.type && ts.isTypeReferenceNode(dtsFn.type) &&
dtsFn.type.typeArguments && dtsFn.type.typeArguments[0] ||
null;
if (!typeParam || isAnyKeyword(typeParam)) {
const ngModule = this.resolveNgModuleReference(fn);
const dtsFile = dtsFn.getSourceFile();
const analysis = analyses.has(dtsFile) ? analyses.get(dtsFile) : [];
analysis.push({declaration: dtsFn, ngModule});
analyses.set(dtsFile, analysis);
// Only when processing the dts files do we need to determine which declaration to update.
if (this.processDts) {
const dtsFn = this.getDtsDeclarationForFunction(fn);
const typeParam = dtsFn.type && ts.isTypeReferenceNode(dtsFn.type) &&
dtsFn.type.typeArguments && dtsFn.type.typeArguments[0] ||
null;
if (!typeParam || isAnyKeyword(typeParam)) {
const ngModule = this.resolveNgModuleReference(fn);
const dtsFile = dtsFn.getSourceFile();
const analysis = analyses.has(dtsFile) ? analyses.get(dtsFile) : [];
analysis.push({declaration: dtsFn, ngModule});
analyses.set(dtsFile, analysis);
}
}
});
});
@@ -149,7 +149,7 @@ export class Transformer {
const decorationAnalyses = decorationAnalyzer.analyzeProgram();

const moduleWithProvidersAnalyzer =
bundle.dts && new ModuleWithProvidersAnalyzer(reflectionHost, referencesRegistry);
new ModuleWithProvidersAnalyzer(reflectionHost, referencesRegistry, bundle.dts !== null);
const moduleWithProvidersAnalyses = moduleWithProvidersAnalyzer &&
moduleWithProvidersAnalyzer.analyzeProgram(bundle.src.program);

@@ -338,7 +338,8 @@ runInEachFileSystem(() => {
new MockLogger(), false, program.getTypeChecker(), dtsProgram);
referencesRegistry = new NgccReferencesRegistry(host);

const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry);
const processDts = true;
const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry, processDts);
analyses = analyzer.analyzeProgram(program);
});

@@ -427,21 +428,19 @@ runInEachFileSystem(() => {

describe('tracking references when generic types already present', () => {
let _: typeof absoluteFrom;
let analyses: ModuleWithProvidersAnalyses;
let program: ts.Program;
let dtsProgram: BundleProgram;
let referencesRegistry: NgccReferencesRegistry;
let TEST_DTS_PROGRAM: TestFile[];
let TEST_PROGRAM: TestFile[];

beforeEach(() => {
_ = absoluteFrom;

const TEST_PROGRAM: TestFile[] = [
TEST_PROGRAM = [
{
name: _('/node_modules/test-package/src/entry-point.js'),
contents: `
export * from './explicit';
export * from './module';
`
`,
},
{
name: _('/node_modules/test-package/src/explicit.js'),
@@ -469,26 +468,26 @@ runInEachFileSystem(() => {
};
}
}
`
`,
},
{
name: _('/node_modules/test-package/src/module.js'),
contents: `
export class ExternalModule {}
`
`,
},
{
name: _('/node_modules/some-library/index.d.ts'),
contents: 'export declare class LibraryModule {}'
contents: 'export declare class LibraryModule {}',
},
];
const TEST_DTS_PROGRAM: TestFile[] = [
TEST_DTS_PROGRAM = [
{
name: _('/node_modules/test-package/typings/entry-point.d.ts'),
contents: `
export * from './explicit';
export * from './module';
`
`,
},
{
name: _('/node_modules/test-package/typings/explicit.d.ts'),
@@ -502,13 +501,13 @@ runInEachFileSystem(() => {
static explicitExternalMethod(): ModuleWithProviders<ExternalModule>;
static explicitLibraryMethod(): ModuleWithProviders<LibraryModule>;
}
`
`,
},
{
name: _('/node_modules/test-package/typings/module.d.ts'),
contents: `
export declare class ExternalModule {}
`
`,
},
{
name: _('/node_modules/test-package/typings/core.d.ts'),
@@ -522,29 +521,31 @@ runInEachFileSystem(() => {
ngModule: Type<T>
providers?: Provider[]
}
`
`,
},
{
name: _('/node_modules/some-library/index.d.ts'),
contents: 'export declare class LibraryModule {}'
contents: 'export declare class LibraryModule {}',
},
];
loadTestFiles(TEST_PROGRAM);
loadTestFiles(TEST_DTS_PROGRAM);
});

it('should track references even when nothing needs to be updated', () => {
const bundle = makeTestEntryPointBundle(
'test-package', 'esm2015', false, getRootFiles(TEST_PROGRAM),
getRootFiles(TEST_DTS_PROGRAM));
program = bundle.src.program;
dtsProgram = bundle.dts !;
const program = bundle.src.program;
const dtsProgram = bundle.dts !;
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), dtsProgram);
referencesRegistry = new NgccReferencesRegistry(host);
const referencesRegistry = new NgccReferencesRegistry(host);

const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry);
analyses = analyzer.analyzeProgram(program);
});
const processDts = true;
const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry, processDts);
const analyses = analyzer.analyzeProgram(program);

it('should track references even when nothing needs to be updated', () => {
const file = getSourceFileOrError(
dtsProgram.program, _('/node_modules/test-package/typings/explicit.d.ts'));
expect(analyses.has(file)).toBe(false);
@@ -563,5 +564,34 @@ runInEachFileSystem(() => {
expect(declarations.has(externalModuleDeclaration.name !)).toBe(true);
expect(declarations.has(libraryModuleDeclaration.name !)).toBe(false);
});

it('should track references even when typings have already been processed', () => {
const bundle =
makeTestEntryPointBundle('test-package', 'esm2015', false, getRootFiles(TEST_PROGRAM));
const program = bundle.src.program;
const host =
new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker(), null);
const referencesRegistry = new NgccReferencesRegistry(host);

const processDts = false; // Emulate the scenario where typings have already been processed
const analyzer = new ModuleWithProvidersAnalyzer(host, referencesRegistry, processDts);
const analyses = analyzer.analyzeProgram(program);

expect(analyses.size).toBe(0);

const declarations = referencesRegistry.getDeclarationMap();
const explicitInternalModuleDeclaration = getDeclaration(
program, absoluteFrom('/node_modules/test-package/src/explicit.js'),
'ExplicitInternalModule', ts.isClassDeclaration);
const externalModuleDeclaration = getDeclaration(
program, absoluteFrom('/node_modules/test-package/src/module.js'), 'ExternalModule',
ts.isClassDeclaration);
const libraryModuleDeclaration = getDeclaration(
program, absoluteFrom('/node_modules/some-library/index.d.ts'), 'LibraryModule',
ts.isClassDeclaration);
expect(declarations.has(explicitInternalModuleDeclaration.name !)).toBe(true);
expect(declarations.has(externalModuleDeclaration.name !)).toBe(true);
expect(declarations.has(libraryModuleDeclaration.name !)).toBe(false);
});
});
});
@@ -175,6 +175,54 @@ runInEachFileSystem(() => {
expect(jsContents).toMatch(/\bvar _c0 =/);
});

it('should add generic type for ModuleWithProviders and generate exports for private modules',
() => {
compileIntoApf('test-package', {
'/index.ts': `
import {ModuleWithProviders} from '@angular/core';
import {InternalFooModule} from './internal';
export class FooModule {
static forRoot(): ModuleWithProviders {
return {
ngModule: InternalFooModule,
};
}
}
`,
'/internal.ts': `
import {NgModule} from '@angular/core';
@NgModule()
export class InternalFooModule {}
`,
});

mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: 'test-package',
propertiesToConsider: ['esm2015', 'esm5', 'module'],
});

// The .d.ts where FooModule is declared should have a generic type added
const dtsContents = fs.readFile(_(`/node_modules/test-package/src/index.d.ts`));
expect(dtsContents).toContain(`import * as ɵngcc0 from './internal';`);
expect(dtsContents)
.toContain(`static forRoot(): ModuleWithProviders<ɵngcc0.InternalFooModule>`);

// The public facing .d.ts should export the InternalFooModule
const entryDtsContents = fs.readFile(_(`/node_modules/test-package/index.d.ts`));
expect(entryDtsContents).toContain(`export {InternalFooModule} from './src/internal';`);

// The esm2015 index source should export the InternalFooModule
const esm2015Contents = fs.readFile(_(`/node_modules/test-package/esm2015/index.js`));
expect(esm2015Contents).toContain(`export {InternalFooModule} from './src/internal';`);

// The esm5 index source should also export the InternalFooModule
const esm5Contents = fs.readFile(_(`/node_modules/test-package/esm5/index.js`));
expect(esm5Contents).toContain(`export {InternalFooModule} from './src/internal';`);
});

describe('in async mode', () => {
it('should run ngcc without errors for fesm2015', async() => {
const promise = mainNgcc({
@@ -76,7 +76,8 @@ function createTestRenderer(
const decorationAnalyses =
new DecorationAnalyzer(fs, bundle, host, referencesRegistry).analyzeProgram();
const moduleWithProvidersAnalyses =
new ModuleWithProvidersAnalyzer(host, referencesRegistry).analyzeProgram(bundle.src.program);
new ModuleWithProvidersAnalyzer(host, referencesRegistry, true)
.analyzeProgram(bundle.src.program);
const privateDeclarationsAnalyses =
new PrivateDeclarationsAnalyzer(host, referencesRegistry).analyzeProgram(bundle.src.program);
const testFormatter = new TestRenderingFormatter();
@@ -574,7 +574,7 @@ export { D };

const referencesRegistry = new NgccReferencesRegistry(host);
const moduleWithProvidersAnalyses =
new ModuleWithProvidersAnalyzer(host, referencesRegistry)
new ModuleWithProvidersAnalyzer(host, referencesRegistry, true)
.analyzeProgram(bundle.src.program);
const typingsFile = getSourceFileOrError(bundle.dts !.program, _('/typings/index.d.ts'));
const moduleWithProvidersInfo = moduleWithProvidersAnalyses.get(typingsFile) !;
@@ -610,7 +610,7 @@ export { D };

const referencesRegistry = new NgccReferencesRegistry(host);
const moduleWithProvidersAnalyses =
new ModuleWithProvidersAnalyzer(host, referencesRegistry)
new ModuleWithProvidersAnalyzer(host, referencesRegistry, true)
.analyzeProgram(bundle.src.program);
const typingsFile =
getSourceFileOrError(bundle.dts !.program, _('/typings/module.d.ts'));

0 comments on commit f3d8f6a

Please sign in to comment.
You can’t perform that action at this time.