Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance optimizations #1764

Merged
merged 6 commits into from
Oct 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 14 additions & 5 deletions integration/watch/intra-dependent.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,27 +22,27 @@ describe('intra-dependent', () => {
harness.expectFesm2015ToMatch('intra-dependent-secondary', /count = 100/);
});

it('should throw error component inputs is changed without updating usages', (done) => {
it('should throw error component inputs is changed without updating usages', done => {
harness.copyTestCase('invalid-component-property');

harness.onFailure((error) => {
harness.onFailure(error => {
expect(error.message).to.match(/Can\'t bind to \'count\' since it isn\'t a known property/);
harness.copyTestCase('valid');
done();
});
});

it('should throw error service method is changed without updating usages', (done) => {
it('should throw error service method is changed without updating usages', done => {
harness.copyTestCase('invalid-service-method');

harness.onFailure((error) => {
harness.onFailure(error => {
expect(error.message).to.match(/Property \'initialize\' does not exist on type \'PrimaryAngularService\'/);
harness.copyTestCase('valid');
done();
});
});

it('should only build entrypoints that are dependent on the file changed.', (done) => {
it('should only build entrypoints that are dependent on the file changed.', done => {
const primaryFesmPath = harness.getFilePath('fesm2015/intra-dependent.js');
const secondaryFesmPath = harness.getFilePath('fesm2015/intra-dependent-secondary.js');
const thirdFesmPath = harness.getFilePath('fesm2015/intra-dependent-third.js');
Expand All @@ -59,4 +59,13 @@ describe('intra-dependent', () => {
done();
});
});

it('should fail when introducing a circular import.', done => {
harness.copyTestCase('circular');

harness.onFailure(error => {
expect(error.message).to.contain('Entry point intra-dependent has a circular dependency on itself.');
done();
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import { Injectable } from '@angular/core';
import 'intra-dependent';

@Injectable()
export class PrimaryAngularService {
initialize() {
// stub
}
}
45 changes: 3 additions & 42 deletions src/lib/ng-package/discover-packages.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import * as ajv from 'ajv';
import { pathExistsSync, lstat } from 'fs-extra';
import * as path from 'path';
import * as log from '../utils/log';
import { ensureUnixPath } from '../utils/path';
import { NgEntryPoint } from './entry-point/entry-point';
import { NgPackage } from './package';
import { globFiles } from '../utils/glob';

const ngPackageSchemaJson = require('../../ng-package.schema.json');
import { validateNgPackageSchema } from './schema';

interface UserPackage {
/** Values from the `package.json` file of this user package. */
Expand All @@ -18,19 +16,6 @@ interface UserPackage {
basePath: string;
}

function formatSchemaValidationErrors(errors: ajv.ErrorObject[]): string {
return errors
.map(err => {
let message = `Data path ${JSON.stringify(err.dataPath)} ${err.message}`;
if (err.keyword === 'additionalProperties') {
message += ` (${(err.params as any).additionalProperty})`;
}

return message + '.';
})
.join('\n');
}

/**
* Resolves a user's package by testing for 'package.json', 'ng-package.json', or 'ng-package.js'.
*
Expand Down Expand Up @@ -67,36 +52,12 @@ async function resolveUserPackage(folderPathOrFilePath: string, isSecondary = fa
}

if (ngPackageJson) {
const _ajv = ajv({
schemaId: 'auto',
useDefaults: true,
jsonPointers: true,
});

const validate = _ajv.compile(ngPackageSchemaJson);
// Add handler for x-deprecated fields
_ajv.addKeyword('x-deprecated', {
validate: (schema, _data, _parentSchema, _dataPath, _parentDataObject, propertyName) => {
if (schema) {
log.warn(`Option "${propertyName}" is deprecated${typeof schema == 'string' ? ': ' + schema : '.'}`);
}

return true;
},
errors: false,
});

const isValid = validate(ngPackageJson);
if (!isValid) {
throw new Error(
`Configuration doesn\'t match the required schema.\n${formatSchemaValidationErrors(validate.errors)}`,
);
}
validateNgPackageSchema(ngPackageJson);

return {
basePath,
packageJson: packageJson || {},
ngPackageJson: ngPackageJson as Record<string, any>,
ngPackageJson,
};
}

Expand Down
4 changes: 2 additions & 2 deletions src/lib/ng-package/entry-point/compile-ngc.transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,12 @@ export const compileNgcTransform: Transform = transformFromPromise(async graph =

// Compile TypeScript sources
const { esm2015, declarations } = entryPoint.data.destinationFiles;
const { moduleResolutionCache } = entryPoint.cache;
const { moduleResolutionCache, ngccProcessingCache } = entryPoint.cache;
const { basePath, cssUrl, styleIncludePaths } = entryPoint.data.entryPoint;
const stylesheetProcessor = new StylesheetProcessor(basePath, cssUrl, styleIncludePaths);

const ngccProcessor = tsConfig.options.enableIvy
? new NgccProcessor(tsConfig.project, tsConfig.options, entryPoints)
? new NgccProcessor(ngccProcessingCache, tsConfig.project, tsConfig.options, entryPoints)
: undefined;

await compileSourceFiles(
Expand Down
25 changes: 25 additions & 0 deletions src/lib/ng-package/ngcc-cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/**
* Registers the absolute specifiers of libraries that have been processed by ngcc. This cache is
* reused across all entry-points of a package, so module requests across the entry-points can
* determine whether invoking ngcc is necessary.
*
* The cost of invoking ngcc for an entry-point that has already been processed is limited due to
* a fast path in ngcc, however even in this fast-path does ngcc scan the entry-point to determine
* if all dependencies have been processed. This cache allows to avoid that work, as entry-points
* are processed in batches during which the `node_modules` directory is not mutated.
*/
export class NgccProcessingCache {
Copy link
Member

@alan-agius4 alan-agius4 Oct 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Why not simply use a Set directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the documentation aspect of having the dedicated class (JS docblock, method names, etc.). It also provides type-safety with respect to other Set instances, so that's nice as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you have a point with docs for methods which are not added here :P.

I don't think it adds anything from a type-safety perspective, both the below are equity type-safe, unless I am missing something.

const ngccProcessingCache = new Set<string>();
const ngccProcessingCache = new NgccProcessingCache();

I don't have a very strong option about this and you can keep it as is. But I do think that in this case using a class doesn't add any value, especially for an internal implementation on top of that which is used in a single place.

Also, while in this case they are not in use, wrapping the Set with a Class has also some disadvantages where you don't expose all the methods and properties available for it. Example, if you need to use .size or entries, you'd need to implement another method like such https://github.com/ng-packagr/ng-packagr/blob/master/src/lib/file-system/file-cache.ts

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I meant by type-safety is the fact that two Sets can easily be interchanged, which is not the case with two distinct implementations. I phrased the earlier comment quite poorly, hopefully this makes more sense. It's similar to the AbsoluteFsPath branded type that's used in the compiler; although it is a string it does offer identification/documentation and more type-safety compared to just using a string.

While you have a point with docs for methods which are not added here :P.

😅 I was mainly referring to the docblock on the class itself.

I see what you're saying from an internal implementation perspective and the fact that this thing is used in so few places. I still like the insight in brings when just glancing over the code, and e.g. the ability to ask an IDE where things are marked as processed (which wouldn't be as easy if this were just Set).

private readonly processedModuleNames = new Set<string>();

hasProcessed(moduleName: string): boolean {
return this.processedModuleNames.has(moduleName);
}

markProcessed(moduleName: string): void {
this.processedModuleNames.add(moduleName);
}

clear(): void {
this.processedModuleNames.clear();
}
}
24 changes: 16 additions & 8 deletions src/lib/ng-package/nodes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { ParsedConfiguration, Program } from '@angular/compiler-cli';
import { Node } from '../graph/node';
import { by, isInProgress, isDirty } from '../graph/select';
import { NgEntryPoint, DestinationFiles } from './entry-point/entry-point';
import { NgccProcessingCache } from './ngcc-cache';
import { NgPackage } from './package';
import { FileCache } from '../file-system/file-cache';
import { ComplexPredicate } from '../graph/build-graph';
Expand Down Expand Up @@ -60,25 +61,30 @@ export function ngUrl(path: string): string {
export class EntryPointNode extends Node {
readonly type = TYPE_NG_ENTRY_POINT;

constructor(public readonly url: string, readonly sourcesFileCache?: FileCache) {
constructor(
public readonly url: string,
sourcesFileCache: FileCache,
ngccProcessingCache: NgccProcessingCache,
moduleResolutionCache: ts.ModuleResolutionCache,
) {
super(url);

if (sourcesFileCache) {
this.cache.sourcesFileCache = sourcesFileCache;
}
this.cache = {
sourcesFileCache,
ngccProcessingCache,
analysesSourcesFileCache: new FileCache(),
moduleResolutionCache,
};
}

cache: {
oldPrograms?: Record<ts.ScriptTarget | 'analysis', Program | ts.Program>;
sourcesFileCache: FileCache;
ngccProcessingCache: NgccProcessingCache;
analysesSourcesFileCache: FileCache;
moduleResolutionCache: ts.ModuleResolutionCache;
rollupFESMCache?: RollupCache;
rollupUMDCache?: RollupCache;
} = {
sourcesFileCache: new FileCache(),
analysesSourcesFileCache: new FileCache(),
moduleResolutionCache: ts.createModuleResolutionCache(process.cwd(), s => s),
};

data: {
Expand All @@ -95,5 +101,7 @@ export class PackageNode extends Node {
cache = {
globCache: {} as GlobCache,
sourcesFileCache: new FileCache(),
ngccProcessingCache: new NgccProcessingCache(),
moduleResolutionCache: ts.createModuleResolutionCache(process.cwd(), s => s),
};
}
39 changes: 27 additions & 12 deletions src/lib/ng-package/package.transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,12 @@ export const packageTransformFactory = (
const ngPkg: PackageNode = foundNode;
const entryPoints = [ngPkg.data.primary, ...ngPkg.data.secondaries].map(entryPoint => {
const { destinationFiles, moduleId } = entryPoint;
const node = new EntryPointNode(ngUrl(moduleId), ngPkg.cache.sourcesFileCache);
const node = new EntryPointNode(
ngUrl(moduleId),
ngPkg.cache.sourcesFileCache,
ngPkg.cache.ngccProcessingCache,
ngPkg.cache.moduleResolutionCache,
);
node.data = { entryPoint, destinationFiles };
node.state = 'dirty';
ngPkg.dependsOn(node);
Expand Down Expand Up @@ -134,9 +139,11 @@ const watchTransformFactory = (
return createFileWatch(data.src, [data.dest]).pipe(
tap(fileChange => {
const { filePath, event } = fileChange;
const { sourcesFileCache } = cache;
const { sourcesFileCache, ngccProcessingCache } = cache;
const cachedSourceFile = sourcesFileCache.get(filePath);

ngccProcessingCache.clear();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to clear the cache on each file file? We don't watch node_modules therefore I assume that we shouldn't do this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to avoid a situation where node_modules was somehow affected between watch builds, as in that case the cache would incorrectly cause ngcc not to run. Even if we don't watch node_modules it's still possible for someone to change a file that is watched after something in node_modules has changed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will not work because of the "local" processed files cache processedModules

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NgccProcessor is recreated in the transform pipeline, so it's not a persistent cache per entry-point.


if (!cachedSourceFile) {
if (event === 'unlink' || event === 'add') {
cache.globCache = regenerateGlobCache(sourcesFileCache);
Expand All @@ -147,27 +154,35 @@ const watchTransformFactory = (
const { declarationFileName } = cachedSourceFile;

const uriToClean = [filePath, declarationFileName].map(x => fileUrl(ensureUnixPath(x)));
let nodesToClean = graph.filter(node => uriToClean.some(uri => uri === node.url));
const nodesToClean = graph.filter(node => uriToClean.some(uri => uri === node.url));

nodesToClean = flatten([
...nodesToClean,
// if a non ts file changes we need to clean up it's direct dependees
// this is mainly done for resources such as html and css
...nodesToClean.filter(x => !x.url.endsWith('.ts')).map(x => x.dependees),
]);
const allUrlsToClean = new Set<string>(
flatten([
...nodesToClean.map(node => node.url),
// if a non ts file changes we need to clean up its direct dependees
// this is mainly done for resources such as html and css
...nodesToClean
.filter(node => !node.url.endsWith('.ts'))
.map(node => node.dependees.map(dependee => dependee.url)),
]),
);

// delete node that changes
nodesToClean.forEach(node => {
sourcesFileCache.delete(fileUrlPath(node.url));
allUrlsToClean.forEach(url => {
sourcesFileCache.delete(fileUrlPath(url));
});

const entryPoints: EntryPointNode[] = graph.filter(isEntryPoint);
entryPoints.forEach(entryPoint => {
const isDirty = entryPoint.dependents.some(x => nodesToClean.some(node => node.url === x.url));
const isDirty = entryPoint.dependents.some(dependent => allUrlsToClean.has(dependent.url));
if (isDirty) {
entryPoint.state = 'dirty';
const { metadata } = entryPoint.data.destinationFiles;
sourcesFileCache.delete(metadata);

uriToClean.forEach(url => {
entryPoint.cache.analysesSourcesFileCache.delete(fileUrlPath(url));
});
}
});

Expand Down
67 changes: 67 additions & 0 deletions src/lib/ng-package/schema.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import * as ajv from 'ajv';
import { NgPackageConfig } from '../../ng-package.schema';
import * as log from '../utils/log';

/** Lazily initialized ajv validator instance. */
let ajvValidator: ajv.ValidateFunction | null = null;
JoostK marked this conversation as resolved.
Show resolved Hide resolved

/**
* Validates the `ngPackageJson` value against the JSON schema using ajv. An error is thrown if
* schema errors are found.
*
* @param ngPackageJson The value to validate.
*/
export function validateNgPackageSchema(ngPackageJson: unknown): asserts ngPackageJson is NgPackageConfig {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

const validate = getNgPackageSchemaValidator();
const isValid = validate(ngPackageJson);
if (!isValid) {
throw new Error(
`Configuration doesn't match the required schema.\n${formatSchemaValidationErrors(validate.errors)}`,
);
}
}

function formatSchemaValidationErrors(errors: ajv.ErrorObject[]): string {
return errors
.map(err => {
let message = `Data path ${JSON.stringify(err.dataPath)} ${err.message}`;
if (err.keyword === 'additionalProperties') {
message += ` (${(err.params as any).additionalProperty})`;
}

return message + '.';
})
.join('\n');
}

/**
* Returns an initialized ajv validator for the ng-package JSON schema.
*/
function getNgPackageSchemaValidator(): ajv.ValidateFunction {
if (ajvValidator !== null) {
JoostK marked this conversation as resolved.
Show resolved Hide resolved
return ajvValidator;
}

const _ajv: ajv.Ajv = ajv({
schemaId: 'auto',
useDefaults: true,
jsonPointers: true,
});

const ngPackageSchemaJson = require('../../ng-package.schema.json');
ajvValidator = _ajv.compile(ngPackageSchemaJson);

// Add handler for x-deprecated fields
_ajv.addKeyword('x-deprecated', {
validate: (schema, _data, _parentSchema, _dataPath, _parentDataObject, propertyName) => {
if (schema) {
log.warn(`Option "${propertyName}" is deprecated${typeof schema == 'string' ? ': ' + schema : '.'}`);
}

return true;
},
errors: false,
});

return ajvValidator;
}
Loading