-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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): support host directives for local compilation mode #53877
Conversation
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.
LGTM, but @crisbeto built host directives and might have some thoughts too
packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/metadata/src/host_directives_resolver.ts
Outdated
Show resolved
Hide resolved
packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts
Outdated
Show resolved
Hide resolved
if (current.directive instanceof Expression || | ||
!ts.isClassDeclaration(current.directive.node)) { | ||
// The case of current.directive being Expression only happens in local compilation mode, | ||
// where we don't want to do any type checking. |
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 still feels non-ideal to me. I wonder, do you think we can do something like this here?
if (localCompilation && !isReference(current.directive)) {
// skip intentional
} else if (!isReference) {
throw new Error("Unexpected")
}
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.
Did something similar by using the narrower type HostDirectiveMetaForGlobalMode
. Ideally we should use this type in all type checking utilities.
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.
Ah no, didn't work. Ended up having to change lot of things. For for now I just throw exception if the host dir meta is not the right type. Having the condition "localCompilation" requires some plumbing of the compilation mode into several classes in the /typecheck/... which makes a footprint rather large for this PR. So I think having a robust solution needs a separate PR and possibly these steps:
- We need to ban the whole code path in /typecheck/... for local compilation. this can be done by checking the compilstion mode in the entrypoints
- Using the type
HostDirectiveMetaForGlobalMode
forTypeCheckableDirectiveMeta.hostDirectives
instead ofHostDirectiveMeta
(tried to do that in this PR but triggered a large cascade changes all over the place)
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.
Why did having a branded expression type not work? e.g.
class LocalCompilationExpression {
__localCompilationBrand = true;
constructor(public expression: o.Expression) {}
}
that could then be passed around in HostDirectiveMeta
and used for reliable checking and type narrowing. Your current solution is better than before, but I think this would be even safer? maybe a follow-up consideration yes
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.
Yeah, make sense. We do have this situation (i.e., | Expression
for local compilation case) in several other places for meta data interfaces. We can probably migrate them all in a separate PR to use a LocalCompilationExpression
(instead of Expression
) and make a contract that any meta with a LocalCompilationExpression type value should be coming from local compilation source, etc. (is my understanding correct?)
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.
Correct, and we can then "filter these" values and throw more safely for "Impossible states"
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.
Gotcha. I add this to my TODO list. For now shall we mark this PR for merge?
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.
Sure!
packages/compiler-cli/src/ngtsc/metadata/src/host_directives_resolver.ts
Outdated
Show resolved
Hide resolved
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.
LGTM once Paul's feedback is addressed.
8616adb
to
aa34263
Compare
At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it.
This PR was merged into the repository by commit 3e13840. |
angular#53877) At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it. PR Close angular#53877
angular#53877) At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it. PR Close angular#53877
angular#53877) At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it. PR Close angular#53877
angular#53877) At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it. PR Close angular#53877
angular#53877) At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it. PR Close angular#53877
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. |
At the moment local compilation breaks for host directives because the current logic relies on global static analysis. This change creates a local version by cutting the diagnostics and copying the directive identifier as it is to the generated code without attempting to statically resolve it.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information