-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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): generate proper reexports in .ngfactory.ts
file…
#13524
Conversation
9a25fec
to
c402ab2
Compare
@@ -21,5 +21,6 @@ export interface PlatformReflectionCapabilities { | |||
method(name: string): MethodFn; | |||
importUri(type: Type<any>): string; | |||
resolveIdentifier(name: string, moduleUrl: string, runtime: any): any; | |||
getIdentifier(name: string, filePath: string, runtime: any): any; |
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 this, pass SaticSymbolCache
to CompileMetadataResolver
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
@@ -42,6 +42,9 @@ export class StaticAndDynamicReflectionCapabilities { | |||
setter(name: string): SetterFn { return this.dynamicDelegate.setter(name); } | |||
method(name: string): MethodFn { return this.dynamicDelegate.method(name); } | |||
importUri(type: any): string { return this.staticDelegate.importUri(type); } | |||
getIdentifier(name: string, filePath: string, runtime: any): any { |
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 this
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
if (!name) { | ||
return null; | ||
} | ||
let filePath: string; | ||
let isFunctionParam: boolean; |
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 variable
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
if (!isFunctionParam) { | ||
filePath = sourceSymbol.filePath; | ||
if (topLevelSymbolNames.has(name)) { |
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.
move logic below into here, i.e. use early returns as well.
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
|
||
// Remove extra underscore from escaped identifier | ||
export function unescapeIdentifier(identifier: string): string { | ||
return identifier.length >= 3 && identifier.charCodeAt(0) === UNDERSCORE_CHAR_CODE && |
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.
Maybe use startsWith('___')
?
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
@@ -70,7 +87,13 @@ export class AotSummaryResolver implements SummaryResolver<StaticSymbol> { | |||
} | |||
if (json) { | |||
const readSummaries = deserializeSummaries(this.staticSymbolCache, json); |
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.
destructure immediately
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
summaries: this.processedSummaries, | ||
symbols: this.symbols.map((symbol, index) => { | ||
let importAs: string; | ||
if (this.summaryResolver.isLibraryFile(symbol.filePath)) { | ||
importAs = `${symbol.name}${index}`; |
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.
Add _
in between name and index?
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
try { | ||
this._directiveResolver.resolve(directiveType, false); | ||
} catch (e) { | ||
console.error('>>>>', directiveType); |
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 this!
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
@@ -20,8 +20,7 @@ import {NgModuleProviderAnalyzer} from './provider_analyzer'; | |||
import {ProviderAst} from './template_parser/template_ast'; | |||
|
|||
export class ComponentFactoryDependency { |
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.
Add a comment that this is not read any more, but we migth need it for later features.
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
constructor( | ||
public comp: CompileIdentifierMetadata, public name: string, | ||
public placeholder: CompileIdentifierMetadata) {} | ||
export class ComponentViewDependency { |
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.
Add comment that these are not used right now but migth be in the future
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
serialize([{symbol: srcSymbol, metadata: 1}, {symbol: libSymbol, metadata: 2}], []) | ||
}); | ||
summaryResolver.getSymbolsOf('/src.d.ts'); | ||
expect(summaryResolver.getImportAs(libSymbol)) |
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.
Check that we don't have an importAs
for srcSymbol
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
Reviewed in person with @vicb |
.ngfactory.ts
file….ngfactory.ts
file…
… not need transitive deps for compiling `.ngfactory.ts` files. Note: This checks the constructors of `@Injectable` classes more strictly. E.g this will fail now as the constructor argument has no `@Inject` nor is the type of the argument a DI token. ``` @Injectable() class MyService { constructor(dep: string) {} } ``` Last part of angular#12787 Closes angular#12787
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. |
To merge, this needs cl/142263969, cl/142784802, plus a run of
release_tsc.sh
in G3.Global presubmit is green (besides flakes).