clientSecret can now (once again) be provided as undefined

[Robbie-Microsoft]

Fixed a regression accidentally introduced in Implemented SHA2 Certificate Functionality. clientSecret can now (once again) be provided as undefined.

added unit test

[yuezk]

Hi,

I'm from #7201 and am curious about the root cause since the code has been strongly typed.

Looks like this is a runtime error and the root cause is as @RoelVB said in #7201 (comment).

Though this PR can fix #7201, I'm afraid we may run into similar issues if it is not fixed from the root.

Related:

• Spread of optional properties can result in unexpected runtime errors microsoft/TypeScript#44432
• Incorrect type inferred when using spread operator to override objects with optional properties. microsoft/TypeScript#57408

[maorleger]

We chatted offline a bit. Here's what I think:

• Agree that there's a larger issue here with the spread operator. There is always a disconnect between your defined types and what you actually get at runtime especially when it comes to your external API
• IMO, fixing this correctly requires some additional thought. While I agree that you could fix the cert key specifically by spreading it out ala clientCertificate: { ...DEFAULT_AUTH_OPTIONS.clientCertificate, ...auth.clientCertificate } }, that still requires discipline whenever objects are added to the config and is an easy trap to fall into. Consider creating a separate issue to track the larger fix and linking to it in your PR
• You may also play around with exactOptionalPropertyTypes which may not help your users but it will help you catch errors internally where possible like this playground - again, it's not perfect especially with spread
• I would also check if config.auth.clientCertificate is used anywhere else in the code just to confirm

LGTM for the immediate future, and a separate issue can be created to address the troubles with the spread operator here. That's at least what I would do!

[Robbie-Microsoft]

Thanks @maorleger!

@RoelVB I agree with your analysis about what the root cause is. However, this behavior (root cause) already existed before the regression this PR addresses was introduced.

@yuezk I'm going to keep this PR as is, and create a new GitHub issue for "larger refactoring to remove the ambiguity for deeply nested properties".

[yuezk]

Thanks for explaining this. I agree with your decision.

[Robbie-Microsoft]

Linking the issue I created.