Skip to content
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

Improper use of Intersection Type #419

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

czindel-sap
Copy link

See https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#intersection-types. This is only on occurrence as an example, the pattern repeats in many places in the type definitions

Using & might be convenient while defining types, but leads to ugly situations when using theses types. With Union Types(|) TypeScript would correctly infer the actual type by using a simple if that checks for a specific property or by using a type guard. But like in the given example, everything is merged together and there is no way to differentiate between a entity or context etc. Which is forcing the consumer to do a type cast. The existing NOTE is actually correct, in many situations we end up with never which is not really helpful

In other place there is code like string & { id:string, ...}. I think the intend is not to say that it is a string with additional properties, most of the time it means OR. So using if(typeof myVar === "string"){...} would do the job for a union type. In the if block TypeScript would then treat myVar only as string and the further implementation would be pretty clean.

I would like to use this PR as a starting point for the discussion, doing the actual change might have a bigger impact.

See https://www.typescriptlang.org/docs/handbook/unions-and-intersections.html#intersection-types.

Using `&` might be convenient while defining types, but leads to ugly situations when using theses types. With Union Types(`|`) TypeScript would correctly infer the actual type by using a simple if or and type guard. But like in the given example everything is merged together and there is no way to differentiate between a entity or context etc. Which is forcing the consumer to do a type cast.
@daogrady
Copy link
Contributor

daogrady commented Mar 10, 2025

Hi @czindel-sap ,

thank you for your suggestion.
I entirely agree: union types are the proper approach here, period.
What you are seeing there is in parts how the types grew historically. I once tried to eliminate these intersection types in favour of proper union types, but the types in questions are partially so deeply rooted in referring types, that changing them would incur a herculean effort to not break several other places. Plus it could imply API changes, as users may have to do explicit type guarding where they did not have to before.

What I did for now is change all respective intersection types I found to union types and wrap them in a utility type, so we can at least track all places where intersections are improperly used (see also the note in the referenced file) and hopefully eliminate them over time.

I am more than happy to receive any contributions like yours! We just have to carefully evaluate any change from intersections to unions wrt their effect they have on functions referring to them.

Best,
Daniel

@czindel-sap
Copy link
Author

@daogrady I also think that it will have a big impact if it is changed, that's why I only gave an example 😉

But speaking from the consumer perspective, I would be happy to adjust my coding if I get better coding assistance and type checks in return. In the current situation I get sometimes strange typing issues (also related to updates of @cap-js/cds-types) and I need to dig deeper in the types to find work arounds or simply do as any, which is then as good as having no type information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants