Skip to content

Generate invalid factory for injectable in local compilation mode#52215

Closed
pmvald wants to merge 3 commits intoangular:mainfrom
pmvald:lcm-abs-inj
Closed

Generate invalid factory for injectable in local compilation mode#52215
pmvald wants to merge 3 commits intoangular:mainfrom
pmvald:lcm-abs-inj

Conversation

@pmvald
Copy link
Member

@pmvald pmvald commented Oct 15, 2023 •
edited
Loading

Currently in local compilation mode the factory is generated by moving the type expression as they are into the inject functions. This disregards the case where the type is a type param:

@Injectable()
export class MainService<S> {
  constructor(private someService1: S ) {}
}

where symbol S will be undefined if moved as value to any function. Another example is type only import as directive ctor injection token:

import {type MyService} from './somewhere';

@Directive()
export class MyDirective {         
  constructor(private myService: MyService) {}  
}

This change takes care of these cases by generating invalid factory. This requires to move this logic into the reflection host as it is no longer possible to rely on its outputs which provides limited info for our purpose.

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 Oct 15, 2023
@ngbot ngbot bot modified the milestone: Backlog Oct 15, 2023
@pmvald pmvald changed the title fix(compiler-cli): generate invalid factory for injectable in local c… Generate invalid factory for injectable in local compilation mode Oct 15, 2023
@pmvald pmvald force-pushed the lcm-abs-inj branch 6 times, most recently from f63612f to fcb01be Compare October 18, 2023 01:16
pmvald added 3 commits October 17, 2023 21:50
@pmvald
refactor(compiler-cli): add local compilation logic to reflection hos…
083821e 
…t `getConstructorParameters` method

Currently the reflection host's `getConstructorParameters` method is not aware of the compilation mode, and it generates result mainly assuming the compilation mode is full and we have access to global type info. As a result, its result is not very suitable for local compilation usage, particularly for deciding if a symbol is imported as type or not. This change plumbs a flag `isLocalCompilation` into reflection host to make it aware of the compilation mode.

Also changes made to the logic in the method `getConstructorParameters` so that in local compilation mode:
 - returns NO_VALUE_DECLARATION type value ref only if the type is a type parameter
 - returns local type value ref for any imported symbol, unless the import is type only in which case returns TYPE_ONLY_IMPORT type value ref
@pmvald
fix(compiler-cli): modify getConstructorDependencies helper to work…
8f5d38c 
… with reflection host after the previous change

Now the method `getConstructorDependencies` no longer needs to do any post analysis, and can rely on the reflection host's result to generate ctor params. This will automatically include invalid factories which fix the issue.
@pmvald
refactor(compiler-cli): remove unnecessary compilationMode args
b085a5c 
After previous commits, some `compilationMode` args in some helper functions became unused. In this change those args are cleaned up.
@pmvald pmvald marked this pull request as ready for review October 18, 2023 02:56
@pmvald pmvald requested review from atscott and crisbeto October 18, 2023 02:56
crisbeto
crisbeto approved these changes Oct 19, 2023
View reviewed changes
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

@pmvald pmvald removed the request for review from atscott October 19, 2023 14:41
@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 Oct 19, 2023
@dylhunn
Copy link
Contributor

dylhunn commented Oct 19, 2023

This PR was merged into the repository by commit ae3acca.

@dylhunn dylhunn closed this in 749d4c4 Oct 19, 2023
dylhunn pushed a commit that referenced this pull request Oct 19, 2023
@pmvald @dylhunn
fix(compiler-cli): modify getConstructorDependencies helper to work…
56a76d7 
… with reflection host after the previous change (#52215)

Now the method `getConstructorDependencies` no longer needs to do any post analysis, and can rely on the reflection host's result to generate ctor params. This will automatically include invalid factories which fix the issue.

PR Close #52215
dylhunn pushed a commit that referenced this pull request Oct 19, 2023
@pmvald @dylhunn
refactor(compiler-cli): remove unnecessary compilationMode args (#5…
ae3acca 
…2215)

After previous commits, some `compilationMode` args in some helper functions became unused. In this change those args are cleaned up.

PR Close #52215
@angular-automatic-lock-bot
Copy link

angular-automatic-lock-bot bot commented Nov 19, 2023

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 Nov 19, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@pmvald @ChellappanRajan
refactor(compiler-cli): add local compilation logic to reflection hos…
69935fc 
…t `getConstructorParameters` method (angular#52215)

Currently the reflection host's `getConstructorParameters` method is not aware of the compilation mode, and it generates result mainly assuming the compilation mode is full and we have access to global type info. As a result, its result is not very suitable for local compilation usage, particularly for deciding if a symbol is imported as type or not. This change plumbs a flag `isLocalCompilation` into reflection host to make it aware of the compilation mode.

Also changes made to the logic in the method `getConstructorParameters` so that in local compilation mode:
 - returns NO_VALUE_DECLARATION type value ref only if the type is a type parameter
 - returns local type value ref for any imported symbol, unless the import is type only in which case returns TYPE_ONLY_IMPORT type value ref

PR Close angular#52215
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@pmvald @ChellappanRajan
fix(compiler-cli): modify getConstructorDependencies helper to work…
2b8205a 
… with reflection host after the previous change (angular#52215)

Now the method `getConstructorDependencies` no longer needs to do any post analysis, and can rely on the reflection host's result to generate ctor params. This will automatically include invalid factories which fix the issue.

PR Close angular#52215
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@pmvald @ChellappanRajan
refactor(compiler-cli): remove unnecessary compilationMode args (an…
a323e10 
…gular#52215)

After previous commits, some `compilationMode` args in some helper functions became unused. In this change those args are cleaned up.

PR Close angular#52215
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@crisbeto crisbeto crisbeto approved these changes

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler PullApprove: disable target: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@pmvald @dylhunn @crisbeto