feat(compiler-cli): support host directives for local compilation mode#53877
feat(compiler-cli): support host directives for local compilation mode#53877pmvald wants to merge 1 commit intoangular:mainfrom
Conversation
devversion
left a comment
There was a problem hiding this comment.
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/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
There was a problem hiding this comment.
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.
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.
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
HostDirectiveMetaForGlobalModeforTypeCheckableDirectiveMeta.hostDirectivesinstead 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.
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.
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.
Correct, and we can then "filter these" values and throw more safely for "Impossible states"
There was a problem hiding this comment.
Gotcha. I add this to my TODO list. For now shall we mark this PR for merge?
packages/compiler-cli/src/ngtsc/metadata/src/host_directives_resolver.ts
Outdated
Show resolved
Hide resolved
crisbeto
left a comment
There was a problem hiding this comment.
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
|
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