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
Various compiler refactorings #16832
Conversation
@@ -175,6 +176,25 @@ export class StaticSymbolResolver { | |||
this.importAs.set(sourceSymbol, targetSymbol); | |||
} | |||
|
|||
importExpr(containingFilePath: string, symbol: StaticSymbol, typeParams: o.Type[]|null = null): o.Expression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove from here again!
a7345ec
to
cc3dbfa
Compare
stylesCompileResults.externalStylesheets.forEach((compiledStyleSheet) => { | ||
generatedFiles.push(this._codgenStyles(srcFileUrl, compiledStyleSheet, fileSuffix)); | ||
const componentStylesheet = this._styleCompiler.compileComponent(outputCtx, compMeta); | ||
compMeta.template !.externalStylesheets.forEach((stylesheetMeta) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider a comment for the non-null assert.
export class GeneratedFile { | ||
constructor(public srcFileUrl: string, public genFileUrl: string, public source: string) {} | ||
constructor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't capture the constraint that either that one of source
or stmts
must be supplied but not both. Maybe this might be better:
source: string | null;
stmts: Statement[] | null;
constructor(public srcFileUrl: string, public genFileUrl: string: src: string | Statement[]) {
if (typeof src === 'string) {
this.source = src;
this.stmts = null;
} else {
this.source = null;
this.stmts = src;
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, good point.
OutputAst
contain the moduleName, not the …@@ -436,11 +439,16 @@ class ViewBuilder implements TemplateAstVisitor, LocalResolver { | |||
let queryMatchExprs: o.Expression[] = []; | |||
ast.queryMatches.forEach((match) => { | |||
let valueType: QueryValueType = undefined !; | |||
if (tokenReference(match.value) === resolveIdentifier(Identifiers.ElementRef)) { | |||
if (tokenReference(match.value) === | |||
this.reflector.resolveExternalReference(Identifiers.ElementRef)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we cache the values or relying on the reflector cache is good enough ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather rely on the cache for now to not change too many things.
import {LifecycleHooks as Hooks} from '@angular/core/src/metadata/lifecycle_hooks'; | ||
|
||
function hasLifecycleHook(hook: Hooks, directive: any): boolean { | ||
return hasLifecycleHookImnpl(new JitReflector(), hook, directive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo ? "Impl"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3rd commit LGTM- minor nits
For the last 2 commits: see #16871 |
This was closed due to deleting the |
…filePath (angular#16832). The goal of this change is to simplify the emitters, as we will soon create a new one to emit TypeScript nodes directly.
This is in preparation for creating typescript nodes directly from `OutputAst` nodes.
Using the global reflector made it impossible to compile multiple programs at the same time.
AOT compilation can be executed synchronously now, if the `ReosurceLoader` returns a string directly (and no `Promise`).
…filePath (#16832). The goal of this change is to simplify the emitters, as we will soon create a new one to emit TypeScript nodes directly.
This is in preparation for creating typescript nodes directly from `OutputAst` nodes.
Using the global reflector made it impossible to compile multiple programs at the same time.
…filePath (angular#16832). The goal of this change is to simplify the emitters, as we will soon create a new one to emit TypeScript nodes directly.
This is in preparation for creating typescript nodes directly from `OutputAst` nodes.
Using the global reflector made it impossible to compile multiple programs at the same time.
AOT compilation can be executed synchronously now, if the `ReosurceLoader` returns a string directly (and no `Promise`).
…filePath (angular#16832). The goal of this change is to simplify the emitters, as we will soon create a new one to emit TypeScript nodes directly.
This is in preparation for creating typescript nodes directly from `OutputAst` nodes.
Using the global reflector made it impossible to compile multiple programs at the same time.
AOT compilation can be executed synchronously now, if the `ReosurceLoader` returns a string directly (and no `Promise`).
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
…filePath.
The goal of this change is to simplify the emitters,
as we will soon create a new one to emit TypeScript nodes directly.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: