[AppConfig] Type mismatch for ConfigurationSettingId.label

[Eskibear]

• Package Name: @azure/app-configuration
• Package Version: 1.4.1
• Operating system: Any
• nodejs
  • version: 18.x
• browser
  • name/version:
• typescript
  • version:
• Is the bug related to documentation in
  • README.md
  • source code documentation
  • SDK API docs on https://docs.microsoft.com

Describe the bug
In response, if a setting has no label, value of the label field is null. However in the type definition, the it's string or undefined.

To Reproduce
Steps to reproduce the behavior:

1. create a configuration setting with no label in portal.
2. use client to get the setting, observe the response

Expected behavior
If the value can be null, the type defintion of ConfigurationSettingId.label should explicitly allow null, be like label?: string | null.

Screenshots

Additional context

azure-sdk-for-js/sdk/appconfiguration/app-configuration/src/models.ts

Lines 77 to 81 in d5c6a7a

	/**
	* The label for this setting. Leaving this undefined means this
	* setting does not have a label.
	*/
	label?: string;

Type ConfigurationSettingResponse extends ConfigurationSetting, extends ConfigurationSettingParam, finally extends ConfigurationSettingId...

[minhanh-phan]

@Eskibear Thank you for reporting the issue. I agree that this is inconsistent type for label. Instead of updating the type of label to string | null | undefined because it will be redundant to have both null and undefined, we'll make changes to the response output. The response with label: null output will be label: undefined. So the type of label will not be changed, but the response output will, and we'll publish the mentioned fix soon. cc/ @HarshaNalluru

[Eskibear]

@minhanh-phan Thank you for the quick response!

I agree that we keep string | undefined and don't introduce a null to eliminate confusion, just as described in tslint rule 'no-null-keyword'.

My little concern is whether the change breaks user code. It would work if they loosely check it like if (setting.label), but it might break if there's strict check like if(setting.lable === null) in user code. But anyway, it will change something and I am glad to have the fix you mentioned, which eliminates the inconsistency.

cc/ @zhenlan @juniwang @avanigupta

[minhanh-phan]

@Eskibear Thank you for raising up that point. That is a valid concern that we share. At the same time, based on app configuration public API, the value returned for label properties shouldn't be null, so the clients shouldn't depend on this behavior. We also suspect if there would be any customers that do a strict check for null. In this case, we're fixing a bug to make sure our implementation matches out API contract.

[minhanh-phan]

We have released @azure/app-configuration 1.5.0 with the fix. Please give it a try and let us know if the issue has been resolved. Thank you for raising the issue!

[Eskibear]

it works as expected.