New user requires review about ngrx-forms HOT 6 CLOSED

Hi @sylvaingirardbe, it's always nice to see new people trying out the library.

I like your code, and there isn't much that I would to different. Here are a couple of points:

1. I don't use classes but just plain objects (with an interface) for form values. While classes seem to work just fine, internally ngrx-forms has no logic to preserve the class identity of the object. Instead, it will always copy over the properties into new plain objects. That means formState.value is not an instance of Asset which may cause issues if you use e.g. instanceof or expect the value to have some class method. However, as long as you are aware of these potential issues it's just fine to keep using classes.
2. I noticed that the address validation is not running on the current sub address all the time but only once it is saved as part of the asset. I am not sure if this is what you intended. In any case, you can remove a bit of redundancy by declaring some more (admittedly verbose) helper methods like this. It uses the (a bit unfortunately named) StateUpdateFns interface from ngrx-forms.

const assetAddressFormStateValidationUpdateFns: StateUpdateFns<AssetAddress> = { 
  countryCode: validate(required)
};

const validateAssetAddressFormState = updateGroup(assetAddressFormStateValidationUpdateFns);
const assetAddressFormStateValidationReducer = createFormGroupReducerWithUpdate(assetAddressFormStateValidationUpdateFns);

const assetFormStateValidationUpdateFns: StateUpdateFns<Asset> = { 
  location: updateGroup<AssetLocation>({ 
    address: validateAssetAddressFormState
  })
};

const validateAssetFormState = updateGroup(assetFormStateValidationUpdateFns);
const assetFormStateValidationReducer = createFormGroupReducerWithUpdate(assetFormStateValidationUpdateFns);

// and then in your reducer as you did before:
state = {
  ...state,
  formState: assetFormStateValidationReducer(state.formState, action),
  currentSubaddress: state.currentSubaddress != undefined ? assetAddressFormStateValidationReducer (state.currentSubaddress, action) : undefined
};

3. Not related to ngrx-forms: In addOrReplaceAddressInAssetIdentification you can shorten the code quite a bit by using splice instead of slice. This should do the same:

export const addOrReplaceAddressInAssetIdentification = (asset: Asset, address: AssetAddress): Asset => {
  const addressIndex = asset.identification.addresses.findIndex(c => c.id == address.id);
  const newAddresses = asset.identification.addresses.splice(addressIndex, 1, address);

  return new Asset({
    ...asset,
    identification: new AssetIdentification({
      ...asset.identification,
      addresses: newAddresses
    })
  });
}

As I mentioned above, none of these points are mandatory and your code is fine without these changes. I'll keep this issue open in case you have more questions. Feel free to close it yourself once you are confident you have achieved what you want.

PS: I edited your post to include syntax highlighting which makes it easier to read.

from ngrx-forms.

Ok, thanks a bunch for the feedback. I was aware of No.1. No.2, not so much. I'll update my code a bit, same for No.3.

from ngrx-forms.

I've come across something else now. I can't seem to figure out how to disable a control. The examples about disabling controls are disabling entire control groups and not just specific controls in a group.

Doing something like this doesn't work since disable expects a state.

const disableControls = updateGroup<Asset>({
    identification: updateGroup<AssetIdentification>({
        details: updateGroup({
            fmisCode: disable()
        })
    })
});

from ngrx-forms.

Just leave away the parentheses on disable. disable is already a function of the correct form that can be used as an update function for the control.

from ngrx-forms.

@sylvaingirardbe did this resolve your issue? Can I close this?

from ngrx-forms.

Yep, thanks. I'll reopen when I come across something else.

from ngrx-forms.