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

feat(compiler-cli): lower some exported expressions #30038

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -223,9 +223,19 @@ function isEligibleForLowering(node: ts.Node | undefined): boolean {
// Don't lower expressions in a declaration.
return false;
case ts.SyntaxKind.VariableDeclaration:
// Avoid lowering expressions already in an exported variable declaration
return (ts.getCombinedModifierFlags(node as ts.VariableDeclaration) &
ts.ModifierFlags.Export) == 0;
const isExported = (ts.getCombinedModifierFlags(node as ts.VariableDeclaration) &
ts.ModifierFlags.Export) == 0;
// This might be unnecessary, as the variable might be exported and only used as a reference
// in another expression. However, the variable also might be involved in provider
// definitions. If that's the case, there is a specific token (`ROUTES`) which the compiler
// attempts to understand deeply. Sub-expressions within that token (`loadChildren` for
// example) might also require lowering even if the top-level declaration is already
// properly exported.
const varNode = node as ts.VariableDeclaration;
return isExported || (varNode.initializer !== undefined &&
(ts.isObjectLiteralExpression(varNode.initializer) ||

This comment has been minimized.

Copy link
@alexeagle

alexeagle Apr 22, 2019

Contributor

if we really want to reduce risk of this change, we could special-case only the import() callExpression, right? but if you're comfortable with handling more cases, that's great

This comment has been minimized.

Copy link
@alxhub

alxhub Apr 23, 2019

Author Contributor

It's tricky, because isEligibleForLowering recursively considers the whole statement. Another section of logic determines whether the expression needs lowering (for example, loadChildren: string expressions won't be lowered). This function only sets bounds on which statements are considered in the first place.

ts.isArrayLiteralExpression(varNode.initializer) ||
ts.isCallExpression(varNode.initializer)));
}
return isEligibleForLowering(node.parent);
}
@@ -875,6 +875,42 @@ describe('ngc transformer command-line', () => {
expect(mymoduleSource).toMatch(/ɵ0 = .*foo\(\)/);
});

it('should lower loadChildren in an exported variable expression', () => {
write('mymodule.ts', `
import {Component, NgModule} from '@angular/core';
import {RouterModule} from '@angular/router';
export function foo(): string {
console.log('side-effect');
return 'test';
}
@Component({
selector: 'route',
template: 'route',
})
export class Route {}
export const routes = [
{path: '', pathMatch: 'full', component: Route, loadChildren: foo()}
];
@NgModule({
declarations: [Route],
imports: [
RouterModule.forRoot(routes),
]
})
export class MyModule {}
`);
expect(compile()).toEqual(0);

const mymodulejs = path.resolve(outDir, 'mymodule.js');
const mymoduleSource = fs.readFileSync(mymodulejs, 'utf8');
expect(mymoduleSource).toContain('loadChildren: ɵ0');
expect(mymoduleSource).toMatch(/ɵ0 = .*foo\(\)/);
});

it('should be able to lower supported expressions', () => {
writeConfig(`{
"extends": "./tsconfig-base.json",
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.