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): support host directives for local compilation mode #53877

Closed
wants to merge 1 commit into from

Conversation

pmvald
Copy link
Member

@pmvald pmvald commented Jan 11, 2024

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?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@pmvald pmvald added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release PullApprove: disable labels Jan 11, 2024
@ngbot ngbot bot added this to the Backlog milestone Jan 11, 2024
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Jan 11, 2024
Copy link
Member

@devversion devversion left a 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

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.
Copy link
Member

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")
}

Copy link
Member Author

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.

Copy link
Member Author

@pmvald pmvald Jan 18, 2024

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:

  1. 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
  2. Using the type HostDirectiveMetaForGlobalMode for TypeCheckableDirectiveMeta.hostDirectives instead of HostDirectiveMeta (tried to do that in this PR but triggered a large cascade changes all over the place)

Copy link
Member

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

Copy link
Member Author

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?)

Copy link
Member

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"

Copy link
Member Author

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?

Copy link
Member

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/test/dts_spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@crisbeto crisbeto left a 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.

@pmvald pmvald force-pushed the lcm-host-dir branch 3 times, most recently from 8616adb to aa34263 Compare January 18, 2024 01:41
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.
@pmvald pmvald added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 20, 2024
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 3e13840.

ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
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
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
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
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
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
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
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
nikvarma pushed a commit to nikvarma/angular that referenced this pull request Jan 31, 2024
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-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler detected: feature PR contains a feature commit PullApprove: disable target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants