Skip to content

Commit

Permalink
fix(compiler-cli): use Map rather than object for map of partial …
Browse files Browse the repository at this point in the history
…linkers (#40563)

Previously, we were naïvely checking whether a function name was a partial linker
declaration call by testing the map of linkers with `linkers[name]`. Since
`linkers` was a plain object, it also matched function names like `toString`!

This has been refactored as a `Map` to avoid the problem.

PR Close #40563
  • Loading branch information
petebacondarwin authored and thePunderWoman committed Jan 25, 2021
1 parent da3e280 commit 33e0f2b
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ export const ɵɵngDeclareDirective = 'ɵɵngDeclareDirective';
export const ɵɵngDeclareComponent = 'ɵɵngDeclareComponent';
export const declarationFunctions = [ɵɵngDeclareDirective, ɵɵngDeclareComponent];

interface LinkerRange<TExpression> {
range: string;
linker: PartialLinker<TExpression>;
}

/**
* A helper that selects the appropriate `PartialLinker` for a given declaration.
*
Expand All @@ -35,7 +40,7 @@ export const declarationFunctions = [ɵɵngDeclareDirective, ɵɵngDeclareCompon
* allows the linker to work on local builds effectively.
*/
export class PartialLinkerSelector<TStatement, TExpression> {
private readonly linkers: Record<string, {range: string, linker: PartialLinker<TExpression>}[]>;
private readonly linkers: Map<string, LinkerRange<TExpression>[]>;

constructor(
environment: LinkerEnvironment<TStatement, TExpression>, sourceUrl: AbsoluteFsPath,
Expand All @@ -47,18 +52,18 @@ export class PartialLinkerSelector<TStatement, TExpression> {
* Returns true if there are `PartialLinker` classes that can handle functions with this name.
*/
supportsDeclaration(functionName: string): boolean {
return this.linkers[functionName] !== undefined;
return this.linkers.has(functionName);
}

/**
* Returns the `PartialLinker` that can handle functions with the given name and version.
* Throws an error if there is none.
*/
getLinker(functionName: string, version: string): PartialLinker<TExpression> {
const versions = this.linkers[functionName];
if (versions === undefined) {
if (!this.linkers.has(functionName)) {
throw new Error(`Unknown partial declaration function ${functionName}.`);
}
const versions = this.linkers.get(functionName)!;
for (const {range, linker} of versions) {
if (satisfies(version, range, {includePrerelease: true})) {
return linker;
Expand All @@ -71,21 +76,21 @@ export class PartialLinkerSelector<TStatement, TExpression> {

private createLinkerMap(
environment: LinkerEnvironment<TStatement, TExpression>, sourceUrl: AbsoluteFsPath,
code: string) {
code: string): Map<string, LinkerRange<TExpression>[]> {
const partialDirectiveLinkerVersion1 = new PartialDirectiveLinkerVersion1(sourceUrl, code);
const partialComponentLinkerVersion1 = new PartialComponentLinkerVersion1(
environment, createGetSourceFile(sourceUrl, code, environment.sourceFileLoader), sourceUrl,
code);

return {
[ɵɵngDeclareDirective]: [
{range: '0.0.0-PLACEHOLDER', linker: partialDirectiveLinkerVersion1},
{range: '>=11.1.0-next.1', linker: partialDirectiveLinkerVersion1},
],
[ɵɵngDeclareComponent]: [
{range: '0.0.0-PLACEHOLDER', linker: partialComponentLinkerVersion1},
{range: '>=11.1.0-next.1', linker: partialComponentLinkerVersion1},
],
};
const linkers = new Map<string, LinkerRange<TExpression>[]>();
linkers.set(ɵɵngDeclareDirective, [
{range: '0.0.0-PLACEHOLDER', linker: partialDirectiveLinkerVersion1},
{range: '>=11.1.0-next.1', linker: partialDirectiveLinkerVersion1},
]);
linkers.set(ɵɵngDeclareComponent, [
{range: '0.0.0-PLACEHOLDER', linker: partialComponentLinkerVersion1},
{range: '>=11.1.0-next.1', linker: partialComponentLinkerVersion1},
]);
return linkers;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,12 @@ describe('PartialLinkerSelector', () => {
expect(selector.supportsDeclaration('ɵɵngDeclareComponent')).toBe(true);
expect(selector.supportsDeclaration('$foo')).toBe(false);
});

it('should return false for methods on `Object`', () => {
const selector = new PartialLinkerSelector(
environment, fs.resolve('/some/path/to/file.js'), 'some file contents');
expect(selector.supportsDeclaration('toString')).toBe(false);
});
});

describe('getLinker()', () => {
Expand Down

0 comments on commit 33e0f2b

Please sign in to comment.