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): typed argument for FormBuilder group #26985

Closed
wants to merge 1 commit into
base: master
from

Conversation

@cexbrayat
Copy link
Contributor

cexbrayat commented Nov 7, 2018

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?

The refactored group method of FormBuilder introduced by #24599 allows two types of options but the signature doesn't show it.

What is the new behavior?

It adds a stricter type to the refactored group method.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

cc @kara

@googlebot googlebot added the cla: yes label Nov 7, 2018

@kara kara added the comp: forms label Nov 7, 2018

@ngbot ngbot bot added this to the needsTriage milestone Nov 7, 2018

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/type-safe-group branch from f22fba3 to 1d25da2 Nov 7, 2018

@IgorMinar
Copy link
Member

IgorMinar left a comment

@kara can you take a look too please? Thanks

validators = legacyOrOpts.validators != null ? legacyOrOpts.validators : null;
asyncValidators =
legacyOrOpts.asyncValidators != null ? legacyOrOpts.asyncValidators : null;
updateOn = legacyOrOpts.updateOn != null ? legacyOrOpts.updateOn : undefined;

This comment has been minimized.

@IgorMinar

IgorMinar Nov 8, 2018

Member

Why do we need all the ternary operator checks? Couldn't we use object destructuring instead? It seems that we are doing a lot of work no obvious reason.

This comment has been minimized.

@LayZeeDK

LayZeeDK Nov 12, 2018

Contributor

To support the legacy options of shape

{
  asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null;
  validator?: ValidatorFn | ValidatorFn[] | null;
}

as well as AbstractControlOptions with similar but slightly differently named properties (notice the ending s)

{
  asyncValidators?: AsyncValidatorFn | AsyncValidatorFn[] | null;
  validators?: ValidatorFn | ValidatorFn[] | null;
  updateOn?: 'change' | 'blur' | 'submit';
}
FormGroup {
group(
controlsConfig: {[key: string]: any},
legacyOrOpts: AbstractControlOptions|{[key: string]: any}|null = null): FormGroup {

This comment has been minimized.

@IgorMinar

IgorMinar Nov 8, 2018

Member

I'm now realizing that this param name is very confusing. I think we should just call it options and document that it has two shapes one of which is deprecated.

This comment has been minimized.

@kara

kara Nov 21, 2018

Contributor

Works for me

@@ -205,7 +205,7 @@ export declare class FormBuilder {
control(formState: any, validatorOrOpts?: ValidatorFn | ValidatorFn[] | AbstractControlOptions | null, asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null): FormControl;
group(controlsConfig: {
[key: string]: any;
}, legacyOrOpts?: {
}, legacyOrOpts?: AbstractControlOptions | {

This comment has been minimized.

@IgorMinar

IgorMinar Nov 8, 2018

Member

I like this. Change. At minimum from docs perspective it makes the api cleaner.

This comment has been minimized.

@kara

kara Nov 21, 2018

Contributor

Yeah, in terms of types, there isn't really a difference, but I do think this is helpful documentation.

This comment has been minimized.

@skreborn

skreborn Nov 21, 2018

Contributor

@IgorMinar Should legacyOrOpts be replaced with options here, too?

@IgorMinar IgorMinar requested a review from kara Nov 8, 2018

@LayZeeDK

This comment has been minimized.

Copy link
Contributor

LayZeeDK commented Nov 12, 2018

@kara Note that type declarations were previously made less strict because of internal Google tests.

@kara

kara approved these changes Nov 21, 2018

Copy link
Contributor

kara left a comment

LGTM (once Igor's comments are addressed)

FormGroup {
group(
controlsConfig: {[key: string]: any},
legacyOrOpts: AbstractControlOptions|{[key: string]: any}|null = null): FormGroup {

This comment has been minimized.

@kara

kara Nov 21, 2018

Contributor

Works for me

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/type-safe-group branch from 1d25da2 to fac1fdb Nov 21, 2018

@cexbrayat

This comment has been minimized.

Copy link
Contributor Author

cexbrayat commented Nov 21, 2018

@kara Thank you for your review. I renamed legacyOrOpts to options as requested by @IgorMinar and amended the commit.

@skreborn
Copy link
Contributor

skreborn left a comment

Just casual nitpicking following up on the legacyOrOpts -> options change.

updateOn = legacyOrOpts.updateOn != null ? legacyOrOpts.updateOn : undefined;
if (options != null) {
if (isAbstractControlOptions(options)) {
// `legacyOrOpts` are `AbstractControlOptions`

This comment has been minimized.

@skreborn

skreborn Nov 21, 2018

Contributor

legacyOrOpts should also be replaced in the comment here since it was renamed to options.

This comment has been minimized.

@cexbrayat

cexbrayat Nov 21, 2018

Author Contributor

Right, thanks for catching that. I fixed.

asyncValidators = options.asyncValidators != null ? options.asyncValidators : null;
updateOn = options.updateOn != null ? options.updateOn : undefined;
} else {
// `legacyOrOpts` are legacy form group options

This comment has been minimized.

@skreborn

skreborn Nov 21, 2018

Contributor

Same as above.

@@ -205,7 +205,7 @@ export declare class FormBuilder {
control(formState: any, validatorOrOpts?: ValidatorFn | ValidatorFn[] | AbstractControlOptions | null, asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null): FormControl;
group(controlsConfig: {
[key: string]: any;
}, legacyOrOpts?: {
}, legacyOrOpts?: AbstractControlOptions | {

This comment has been minimized.

@skreborn

skreborn Nov 21, 2018

Contributor

@IgorMinar Should legacyOrOpts be replaced with options here, too?

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/type-safe-group branch from fac1fdb to 00feb89 Nov 21, 2018

@cexbrayat cexbrayat force-pushed the cexbrayat:fix/type-safe-group branch from 00feb89 to 60824ff Nov 21, 2018

@LayZeeDK

This comment has been minimized.

Copy link
Contributor

LayZeeDK commented Nov 21, 2018

Please note my comment and make sure that you don't recreate the issue that broke your internal Google tests, @kara and @IgorMinar
#26985 (comment)

This is the reason types were made less strict and correct.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 21, 2018

@LayZeeDK This PR does not actually make any meaningful type changes because AbstractControlOptions is a subtype of {[key:string]: any} type and does not replace it in the signature. It's mostly being added for documentation purposes.

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 21, 2018

@kara

kara approved these changes Nov 21, 2018

Copy link
Contributor

kara left a comment

LGTM

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 21, 2018

@IgorMinar Can you take another look?

@LayZeeDK

This comment has been minimized.

Copy link
Contributor

LayZeeDK commented Nov 22, 2018

So Angular 7.1.0 landed. This will be in which version now? 7.2.0? 7.1.x?

@IgorMinar
Copy link
Member

IgorMinar left a comment

Lgtm. Thanks for being patient!

@alxhub alxhub closed this in b0c7561 Dec 7, 2018

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment