Skip to content

fix(forms): Allow NonNullableFormBuilder to be injected.#45904

Closed
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:inject-fb
Closed

fix(forms): Allow NonNullableFormBuilder to be injected.#45904
dylhunn wants to merge 1 commit intoangular:mainfrom
dylhunn:inject-fb

Conversation

@dylhunn
Copy link
Contributor

@dylhunn dylhunn commented May 5, 2022

Based on early feedback, fb.nonNullable.group(...) continues to be clunky for a form with many such groups. Allowing NonNullableFormBuilder to be directly injected enables the following:

class Foo {
  constructor(private fb: NonNullableFormBuilder) {
    this.fb.group({foo: 1});
  }
}

@dylhunn dylhunn added area: forms forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. labels May 5, 2022
@ngbot ngbot bot modified the milestone: Backlog May 5, 2022
@dylhunn dylhunn added the action: review The PR is still awaiting reviews from at least one requested reviewer label May 5, 2022
@dylhunn dylhunn requested a review from AndrewKushnir May 5, 2022 23:02
@dylhunn dylhunn marked this pull request as ready for review May 5, 2022 23:02
@pullapprove pullapprove bot requested review from alxhub and jessicajaniuk May 5, 2022 23:03
@dylhunn
Copy link
Contributor Author

dylhunn commented May 5, 2022

@AndrewKushnir I'm actually unsure of how to unit test this, do any suggestions come to mind?

@dylhunn dylhunn added target: rc This PR is targeted for the next release-candidate merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels May 5, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label May 5, 2022
@dylhunn dylhunn removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels May 5, 2022
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented May 5, 2022

I'm actually unsure of how to unit test this, do any suggestions come to mind?

You can add a test like this:

it('should allow NonNullableFormBuilder to be injected', () => {
  @Component({
    standalone: true,
    template: '...',
  })
  class MyComp {
    constructor(public fb: NonNullableFormBuilder) {}
  }

  const fixture = TestBed.createComponent(MyComp);
  fixture.detectChanges();
  expect(fixture.componentInstance.fb).toBeInstanceOf(NonNullableFormBuilder);
}

I'd propose adding a similar test for FormBuilder as well (if we don't have one).

AndrewKushnir
AndrewKushnir reviewed May 5, 2022
View reviewed changes
@dylhunn dylhunn force-pushed the inject-fb branch 3 times, most recently from e8ccb79 to 6ba5fd4 Compare May 9, 2022 18:26
AndrewKushnir
AndrewKushnir approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Looks good 👍 Just left a couple minor comments.

AndrewKushnir
AndrewKushnir previously approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

jessicajaniuk
jessicajaniuk previously approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@jessicajaniuk jessicajaniuk left a comment

Choose a reason for hiding this comment

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

reviewed-for: public-api

@dylhunn dylhunn dismissed stale reviews from jessicajaniuk and AndrewKushnir via c0de830 May 9, 2022 22:59
@dylhunn dylhunn added the action: merge The PR is ready for merge by the caretaker label May 9, 2022
@dylhunn dylhunn added action: presubmit The PR is in need of a google3 presubmit and removed action: merge The PR is ready for merge by the caretaker labels May 9, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented May 9, 2022 •
edited
Loading

Currently running a standard presubmit.

@dylhunn dylhunn added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels May 9, 2022
@dylhunn dylhunn removed the request for review from pkozlowski-opensource May 9, 2022 23:26
@dylhunn dylhunn added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 9, 2022
@dylhunn
Copy link
Contributor Author

dylhunn commented May 9, 2022

merge-assistance: I'm not sure why pullapprove is fighting me, this is good to go

AndrewKushnir
AndrewKushnir previously approved these changes May 9, 2022
View reviewed changes
@dylhunn dylhunn removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels May 9, 2022
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented May 9, 2022

@dylhunn could you please update the code based on #45904 (review)?

@dylhunn
fix(forms): Allow NonNullableFormBuilder to be injected.
5fcde92 
Based on early feedback, calling `fb.nonNullable.group(...)` continues to be clunky for a form with many such groups. Allowing `NonNullableFormBuilder` to be directly injected enables the following:

```
constructor(private fb: NonNullableFormBuilder) {}
```
@dylhunn
Copy link
Contributor Author

dylhunn commented May 9, 2022

@AndrewKushnir Whoops, now fixed.

@dylhunn dylhunn added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label May 9, 2022
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label May 9, 2022
@dylhunn dylhunn removed the request for review from pkozlowski-opensource May 9, 2022 23:55
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented May 10, 2022

This PR was merged into the repository by commit 43ba4ab.

AndrewKushnir pushed a commit that referenced this pull request May 10, 2022
@dylhunn @AndrewKushnir
fix(forms): Allow NonNullableFormBuilder to be injected. (#45904)
5d13e58 
Based on early feedback, calling `fb.nonNullable.group(...)` continues to be clunky for a form with many such groups. Allowing `NonNullableFormBuilder` to be directly injected enables the following:

```
constructor(private fb: NonNullableFormBuilder) {}
```

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

angular-automatic-lock-bot bot commented Jun 10, 2022

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 Jun 10, 2022
@dylhunn dylhunn deleted the inject-fb branch November 30, 2022 20:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir left review comments

+1 more reviewer

@jessicajaniuk jessicajaniuk jessicajaniuk left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

action: merge The PR is ready for merge by the caretaker area: forms forms: Controls API Issues related to AbstractControl, FormControl, FormGroup, FormArray. forms: strictly typed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: rc This PR is targeted for the next release-candidate

Projects

None yet

Milestone

Backlog

Development

Successfully merging this pull request may close these issues.

3 participants

@dylhunn @AndrewKushnir @jessicajaniuk