support exactOptionalPropertyTypes

[quisido]

I don't know how you handle version bumping, so I didn't try. Theoretically, this should be 1.0.10.

Issue #, if available: resolves #23

Description of changes:

Regarding cloudscape-design/components#1108,

the build failed at the documentation step. Fixing the Documenter is a big deal and our team can't afford it at the moment.

Here's the fix.

Details

buildEventInfo requires two pieces of information:

1. metadata on the handler
2. metadata on the reference type

When building event info for union types, it finds the reference type in the union, then grabs the respective metadata: from the handler and from the reference type in the union
For non-union reference types, the same logic still occurs: it grabs the metadata from the handler and from the reference type.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[just-boris]

Hello!

Thanks for sending this contribution!

However, supporting that feature is not only about merging this feature and forget. It will also enforce coding convention for our team.

Before paying the cost of this new convention, we need to have more demand than just a few enthusiasts.

So, both this PR and the issue will remain open, asking for more people and their use-cases

[quisido]

The documenter PR doesn't enforce any convention. It's more of a bug fix that correctly identifies | undefined. Teams are not required to use it, but it unblocks anyone that does. It shouldn't require buy-in to merge.

I would take some issue with calling exact optional properties a convention, when it is a type fix. It's not a code style. It is a bug fix to the language. Not using it blocks teams that accurately define their types, while using it supports both types of teams. It shouldn't be controversial: one configuration unblocks everyone while accurately defining types, and the other configuration blocks users who accurately define their types.

I'm afraid what you will find is not teams arguing for their use case, but that they have already begrudgingly changed their configuration solely to support this library. Failing to support exact optional types is a compile time build failure, not a code style. Anyone using this package has already adjusted their configuration to support it. Others saw that this package doesn't support their configuration and possibly abandoned the library altogether.

The best source for what you are seeking is the TypeScript documentation itself: the reasons for why they added the configuration in the first place. The issue referenced in the PR that added it is microsoft/TypeScript#13195. You see plenty of real world cases and justifications. I'm afraid you and your team would have to self-motivate to read it, as it's unlikely anyone will bring this issue to you. I tried to be exceptional to not only raise the issue but to address it; you'll find others (especially AWS teams) will have simply abandoned their preference in favor of a library that's required.

I can't stress enough that this is less of an issue of the Cloudscape team having to add | undefined to a few dozen lines of code and more of an issue of consumers being required to remove best practices from their projects in order to consume Cloudscape. The true impact of this change is not on the Cloudscape team but on the consumer who is unable to build any Cloudscape project when using conditional props.

Please weigh the cost benefit analysis here in that this change is 100% backwards compatible. If you need more data, I ask you find it (e.g. the linked GitHub issue), as it won't passively comment on this thread.

[just-boris]

The documenter PR doesn't enforce any convention. It's more of a bug fix that correctly identifies | undefined. Teams are not required to use it, but it unblocks anyone that does. It shouldn't require buy-in to merge.

I am not taking this PR alone but also follow up steps to update all existing interfaces to follow the convention, update development documentation for creating new components and write some compatibility tests.

If there are any signs of this property to become popular, we can go this way. I checked microsoft/TypeScript#44421 to predict the future for this mode and people posted some code examples where this new mode makes the code harder to write and read. Our team was not bitten much enough by object spread issues to justify the cost of this flag.

[just-boris]

Some experience from our past – in the beginning of v3.0 development we received a suggestion to use ReadonlyArray<T> instead of Array<T> to enforce immutability of the props in our typings. Similar to the current proposal, that option added extra cost for us to maintain and also some confusion for customers who tried to mix it with Array<T> in their own code.

Past learning suggests that we should take only more or less popular type features or modes

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.10 🎉

Comparison is base (ec7fac7) 95.18% compared to head (9d05144) 95.28%.

❗ Current head 9d05144 differs from pull request most recent head 38aa6ff. Consider uploading reports for the commit 38aa6ff to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   95.18%   95.28%   +0.10%     
==========================================
  Files          15       15              
  Lines         415      424       +9     
  Branches      142      142              
==========================================
+ Hits          395      404       +9     
  Misses         20       20              

Impacted Files	Coverage Δ
src/components/build-definition.ts	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[quisido]

Similar to the current proposal, that option added extra cost for us to maintain

Can you clarify what the extra cost of maintaining this is? This change should have unchanging transpilation and runtime behavior in all instances where you aren't using hasOwnProperty to check for the existence of a prop. I'm not aware of additional maintenance burden for this, but I'd like to know so that I can be mindful of it in future pull requests.

[just-boris]

Now every time, we used to write prop?: Something we will have to write prop?: Something | undefined. This a coding convention change, and in large projects this is costly (not the initial "find-and-replace" execution, but ensuring that all new added interfaces follow the convention)

I'd like to know so that I can be mindful of it in future pull requests

If a pull request changes existing team workflow (adds new coding convention), this is not a pull request which can come from outside that team. Rule of thumb for our case, if it requires documenter modifications - it is a kind of change which will be difficult for the team to accept

[quisido]

Now every time, we used to write prop?: Something we will have to write prop?: Something | undefined. This a coding convention change,

I'd like to clarify that this is not a coding convention change, but a code accuracy change. prop?: Something and prop?: Something | undefined are not equivalent, which is why TypeScript added the compiler option to support strict enforcement of this difference.

Example TypeScript playground

This can be especially important when rendering conditional elements, such as "If the prop exists, render a div that contains it." This is great when prop="my text" but incorrectly renders an empty element when prop={undefined}. This is an error I've seen more than once, so I know its importance to avoid.

I don't see prop?: X -> prop?: X | undefined as particularly burdensome, but you can make that call. I just want to clarify that this is about code accuracy, not convention or style. These two property definitions have different meanings at runtime that can branch into different behaviors.

[just-boris]

Ack that I read your comment, but not sure what else I can add here 🤷

If something changes and exactOptionalPropertyTypes becomes a part of any popular configuration preset or even enabled by default in future Typescript versions, we may reconsider this. But currently it stays as very exotic flag, not worth investing in support