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(forms): formControlName also accepts a number #30606

Closed

Conversation

@cexbrayat
Copy link
Contributor

commented May 22, 2019

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?

Currently, when using a FormArray, most templates look like:

<div formArrayName="tags">
  <div *ngFor="let tag of tagsArray.controls; index as i">
    <input [formControlName]="i">
  </div>
</div>

Here formControlName receives a number whereas its input type is a string.

This is fine for VE and fullTemplateTypeCheck, but not for Ivy which does a more thorough type checking on inputs with fullTemplateTypeCheck enabled and throws Type 'number' is not assignable to type 'string'. It is fixable by using formControlName="{{i}}" but you have to know the difference between a="{{b}}" and [a]="b" and change it all over the application codebase.

What is the new behavior?

This commit relaxes the type of the formControlName input to accept both a string and a number.

Does this PR introduce a breaking change?

  • Yes
  • [] No

Technically this is a breaking change, see @jelbourn comment #30606 (comment)

Other information

This has been discussed with @kara on Slack.

Note that this PR contains two commits:

  • the first one relaxes the input type to string | number by changing it on the base NgControl class
  • the second one reverts #29473 , where @piotrtomiak ran into a similar issue and changed the example in the docs to help users avoid it. It is now no longer necessary, and the example uses [formControlName]="i" again, as all the other examples in the codebase.

Also note that no tests were added as the issue is already caught by our tests when fullTemplateTypeCheck is on, but it is currently disabled until Ivy type-checking is on par with VE (see FW-1004). For example, adding type_check=True in https://github.com/angular/angular/blob/master/packages/examples/forms/BUILD.bazel#L12-L14 surfaces the issue.

@cexbrayat cexbrayat requested review from IgorMinar and angular/fw-forms as code owners May 22, 2019

@googlebot googlebot added the cla: yes label May 22, 2019

@alfaproject

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

I get that this is being done for backwards compatibility and convenience, but it still feels weird to me that an index (number) can be fed into a variable called name :F

@trotyl

This comment has been minimized.

Copy link
Contributor

commented May 23, 2019

I agree with @alfaproject, there should never be a need* to specify keys in looping a FormArray, instead some directive like formControlItem should be introduced to automatically dealing with FormArray index.

*: The only exception is to create a sparse Array deliberately, which is really bad practice.

@matsko matsko added the comp: forms label May 30, 2019

@ngbot ngbot bot added this to the needsTriage milestone May 30, 2019

@kara
kara approved these changes Jun 13, 2019
Copy link
Contributor

left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

@IgorMinar
Copy link
Member

left a comment

lgtm for api changes. I made some suggestions to the api docs.

@cexbrayat cexbrayat force-pushed the cexbrayat:feat/form-control-name-as-number branch 2 times, most recently from 0fa533d to ddccea0 Jun 16, 2019

@cexbrayat cexbrayat force-pushed the cexbrayat:feat/form-control-name-as-number branch 2 times, most recently from 83db7f5 to 5787baa Jun 19, 2019

kara added a commit to kara/angular that referenced this pull request Aug 10, 2019
kara added a commit to kara/angular that referenced this pull request Aug 10, 2019
@kara kara referenced this pull request Aug 10, 2019
kara added a commit that referenced this pull request Aug 10, 2019
kara added a commit that referenced this pull request Aug 10, 2019

@kara kara reopened this Aug 10, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

Re-opening this so we can look into G3 failures. This will need a global TAP presubmit before we try to merge again.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2019

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
feat(forms): formControlName also accepts a number (angular#30606)
This commit relaxes the type of the `formControlName` input to accept both a `string` and a `number`.

Currently, when using a `FormArray`, most templates look like:

```
<div formArrayName="tags">
  <div *ngFor="let tag of tagsArray.controls; index as i">
    <input [formControlName]="i">
  </div>
</div>
```

Here `formControlName` receives a number whereas its input type is a string.

This is fine for VE and `fullTemplateTypeCheck`, but not for Ivy which does a more thorough type checking on inputs with `fullTemplateTypeCheck` enabled and throws `Type 'number' is not assignable to type 'string'`. It is fixable by using `formControlName="{{i}}"` but you have to know the difference between `a="{{b}}"` and `[a]="b"` and change it all over the application codebase. This commit allows the existing code to still type-check.

PR Close angular#30606
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
docs(forms): use a number as input value for formControlName (angular…
…#30606)

PR angular#29473 changed the docs to use a string as the input value of `formControlName`, as it used to only accept a string.
This has been changed, and `formControlName` now accepts a string or a number, so the example in the docs can use a binding as they used to.

PR Close angular#30606
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019

@kara kara closed this in 628b0c1 Aug 13, 2019

kara added a commit that referenced this pull request Aug 13, 2019
docs(forms): use a number as input value for formControlName (#30606)
PR #29473 changed the docs to use a string as the input value of `formControlName`, as it used to only accept a string.
This has been changed, and `formControlName` now accepts a string or a number, so the example in the docs can use a binding as they used to.

PR Close #30606
@Bil0

This comment has been minimized.

Copy link

commented Aug 30, 2019

Hello,

I think we should consider using number as input value for formGroupName and formArrayName too (for more complex/nested forms), for instance:

<form [formGroup]="myForm" (ngSubmit)="onSubmit()">
  <div formArrayName="users">
    <div *ngFor="let user of users.controls; index as i">
      <div [formGroupName]="i">
        <input formControlName="name" placeholder="Name" type="text"/>
        <input formControlName="age" placeholder="age" type="number"/>
      </div>
    </div>
  </div>
</form>
<button (click)="addUser()">Add user</button>

throws Type 'number' is not assignable to type 'string'.

If it's OK, I can open an issue or submit my first pull request.

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

@Bil0 Yes, I think the best is to open an issue, and we'll see what the forms team (@jasonaden and @kara ) thinks about it. If it is worth a PR, you'll have to do something very similar as this one, and I can give you a hand if needed 👍

sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(forms): formControlName also accepts a number (angular#30606)
This commit relaxes the type of the `formControlName` input to accept both a `string` and a `number`.

Currently, when using a `FormArray`, most templates look like:

```
<div formArrayName="tags">
  <div *ngFor="let tag of tagsArray.controls; index as i">
    <input [formControlName]="i">
  </div>
</div>
```

Here `formControlName` receives a number whereas its input type is a string.

This is fine for VE and `fullTemplateTypeCheck`, but not for Ivy which does a more thorough type checking on inputs with `fullTemplateTypeCheck` enabled and throws `Type 'number' is not assignable to type 'string'`. It is fixable by using `formControlName="{{i}}"` but you have to know the difference between `a="{{b}}"` and `[a]="b"` and change it all over the application codebase. This commit allows the existing code to still type-check.

PR Close angular#30606
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
docs(forms): use a number as input value for formControlName (angular…
…#30606)

PR angular#29473 changed the docs to use a string as the input value of `formControlName`, as it used to only accept a string.
This has been changed, and `formControlName` now accepts a string or a number, so the example in the docs can use a binding as they used to.

PR Close angular#30606
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
feat(forms): formControlName also accepts a number (angular#30606)
This commit relaxes the type of the `formControlName` input to accept both a `string` and a `number`.

Currently, when using a `FormArray`, most templates look like:

```
<div formArrayName="tags">
  <div *ngFor="let tag of tagsArray.controls; index as i">
    <input [formControlName]="i">
  </div>
</div>
```

Here `formControlName` receives a number whereas its input type is a string.

This is fine for VE and `fullTemplateTypeCheck`, but not for Ivy which does a more thorough type checking on inputs with `fullTemplateTypeCheck` enabled and throws `Type 'number' is not assignable to type 'string'`. It is fixable by using `formControlName="{{i}}"` but you have to know the difference between `a="{{b}}"` and `[a]="b"` and change it all over the application codebase. This commit allows the existing code to still type-check.

PR Close angular#30606
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
docs(forms): use a number as input value for formControlName (angular…
…#30606)

PR angular#29473 changed the docs to use a string as the input value of `formControlName`, as it used to only accept a string.
This has been changed, and `formControlName` now accepts a string or a number, so the example in the docs can use a binding as they used to.

PR Close angular#30606

@cexbrayat cexbrayat deleted the cexbrayat:feat/form-control-name-as-number branch Sep 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.