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(forms): Allow NonNullableFormBuilder to be injected. #45904

Closed
wants to merge 1 commit into from

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

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

@dylhunn dylhunn force-pushed the inject-fb branch 3 times, most recently from e8ccb79 to 6ba5fd4 Compare May 9, 2022 18:26
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.

packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
packages/forms/test/form_builder_spec.ts Outdated Show resolved Hide resolved
AndrewKushnir
AndrewKushnir previously approved these changes May 9, 2022
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
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 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

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
packages/forms/src/form_builder.ts Outdated Show resolved Hide resolved
@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

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

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

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

AndrewKushnir pushed a commit that referenced this pull request May 10, 2022
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

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.
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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants