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

fix(core): ensure all initializer functions run in an injection context #54761

Closed

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Mar 8, 2024

Ensures that all of the functions intended to be run in initializers are in an injection context. This is a stop-gap until we have a compiler diagnostic for it.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: rc This PR is targeted for the next release-candidate labels Mar 8, 2024
@crisbeto crisbeto requested a review from devversion March 8, 2024 10:05
@crisbeto crisbeto force-pushed the injection-context-initializers branch from fbfb314 to 493b515 Compare March 8, 2024 12:18
@crisbeto crisbeto marked this pull request as ready for review March 8, 2024 12:28
// non-required query results are undefined before we run creation mode on the view queries
const appCmpt = new AppComponent();
expect(appCmpt.divEl()).toBeUndefined();
it('should return undefined if optional query is read in the constructor', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to rework these tests, because they were instantiating components manually.

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. Only would optionally suggest TestBed.runInInjectionContext for shorter tests

@Component({template: ''})
class TestCmp {
input = input<number>(0);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestBed.runInInjectionContext(() => input(0)) also works and is shorter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I'll change the input tests but will keep the query tests as they are since they're kinda tied to a view.

Ensures that all of the functions intended to be run in initializers are in an injection context. This is a stop-gap until we have a compiler diagnostic for it.
@crisbeto crisbeto force-pushed the injection-context-initializers branch from 493b515 to 31bfdb3 Compare March 8, 2024 13:23
@crisbeto crisbeto added state: blocked and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 8, 2024
@crisbeto
Copy link
Member Author

crisbeto commented Mar 8, 2024

Blocked on reworking internal targets that are using the functions incorrectly.

@crisbeto
Copy link
Member Author

Passing TGP, excluding some flakes and after I land a separate cleanup CL.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Mar 12, 2024
@atscott
Copy link
Contributor

atscott commented Mar 12, 2024

This PR was merged into the repository by commit 018f826.

atscott pushed a commit that referenced this pull request Mar 12, 2024
…xt (#54761)

Ensures that all of the functions intended to be run in initializers are in an injection context. This is a stop-gap until we have a compiler diagnostic for it.

PR Close #54761
@atscott atscott closed this in 018f826 Mar 12, 2024
@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 Apr 12, 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 target: rc This PR is targeted for the next release-candidate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants