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

[FEATURE] Refinement of Type Generation in CDS-Typer for Optional / Nullable Fields #87

Closed
tsteckenborn opened this issue Oct 10, 2023 · 7 comments · Fixed by #111
Closed
Labels
feature request New feature or request keepalive Will not be closed by Stale bot

Comments

@tsteckenborn
Copy link

tsteckenborn commented Oct 10, 2023

Question

Background

When using the CDS-Typer tool to generate TypeScript types for SAP CAP CDS services, it's observed that the tool does not explicitly differentiate between fields that can be null and those that can't. This becomes particularly problematic when TypeScript's exactOptionalPropertyTypes is enabled.

Current Behavior

Both the following CDS definitions for getUserDetails() yield the TypeScript type example?: string;, even though one specifies nullability:

function getExample() returns {
    [...]
    example : String;
    [...]
};
function getExample() returns {
    [...]
    example : String null;
    [...]
};

Expected Behavior

The type generation should be more specific. For instance, a more explicit type definition like example?: string | null; would be more informative and compatible with exactOptionalPropertyTypes.

Note

I'm uncertain on the handling of undefined properties. It might also make sense to check whether example?: string | undefined is an option for the first, example?: string | undefined | null for the second case.

@tsteckenborn tsteckenborn added new question Further information is requested labels Oct 10, 2023
@daogrady daogrady added feature request New feature or request keepalive Will not be closed by Stale bot labels Oct 10, 2023
@daogrady daogrady changed the title [QUESTION] Refinement of Type Generation in CDS-Typer for Optional / Nullable Fields [FEATURE REQUEST] Refinement of Type Generation in CDS-Typer for Optional / Nullable Fields Oct 11, 2023
@daogrady daogrady removed the question Further information is requested label Oct 11, 2023
@daogrady daogrady changed the title [FEATURE REQUEST] Refinement of Type Generation in CDS-Typer for Optional / Nullable Fields [FEATURE] Refinement of Type Generation in CDS-Typer for Optional / Nullable Fields Oct 11, 2023
@daogrady daogrady removed the new label Oct 11, 2023
@siarhei-murkou
Copy link
Contributor

Hi @tsteckenborn , thanks for opening this feature request! 👍

Hi @daogrady , could it be also possible to provide TS type without optional keyword ?: at all for CDS fields with not null keyword & without default value? Which is quite helpful also for cds.INSERT where not null CDS fields should be compiled to strict TS types without null/undefined datatype at all.

Also, could you please say approximately in which lib's release we might expect the delivery of this feature? 🙂

@daogrady
Copy link
Contributor

daogrady commented Nov 8, 2023

Hi @siarhei-murkou,

it is currently not explicitly planned, as other more pressing matters need to be addressed first.
Though you are welcome to submit a PR, if you see a solution. 🙂

Regarding :?: there is a CLI option --propertiesOptional false, although its behaviour is not really supported. Not sure if that helps you.

Best,
Daniel

@siarhei-murkou
Copy link
Contributor

Hi @daogrady ,

Thanks, --propertiesOptional false improves the TS validation much stronger. 👍

The only missing part is to compile nullable fields to TS prop propName: ... | null in order to not manually redefine auto-compiled TS classes/types. But as you said, ... | null enhancement is not in your current roadmap.
If so, I'll try to take a look at this lib implementation a bit later and investigate how to add this extra logic 🙂

@tsteckenborn
Copy link
Author

Hi you two,

As both null and undefined signal absent or uninitialized values - although in theory for different reasons - in case you're heading for it @siarhei-murkou, would we strife for ... | null or ... | null | undefined?

@daogrady
Copy link
Contributor

Hi @tsteckenborn ,

just wanted to let you know @siarhei-murkou is currently working on a PR to introduce the nullable behaviour.
The current approach is to have ... | null for all nullable fields. Do you have an opinion on that?

Best,
Daniel

@tsteckenborn
Copy link
Author

tsteckenborn commented Nov 21, 2023

Hi @daogrady ,

Yeah I saw that. I'm currently not quite sure on how I'd interpret the absence of values (indicated by not having not null or mandatory) in CDS. In general both undefined and null indicate the absence of values - even though for different reasons.

With TypeScript recommending enabling the option exactOptionalPropertyTypes I'd rather opt for allowing both if supported by the runtime. That seems to be handled fine when ignoring the types. Yet as said I'm currently not sure about it and tend to think that I think this way because I'd like it and currently need to do some workarounds...

@daogrady
Copy link
Contributor

I had a brief talk about this with the runtime team and they confirmed null as a valid value for every field that was not explicitly marked as not nullable.
I think it makes sense to only go for null, not for undefined at this point.
We basically have three major ways of saying "some property is not attached":

  1. undefined, which basically means "there has never been a value written to this property"
  2. null, which is usually reserved to express "something was assigned to the property, and it was nothing"
  3. ... ?: ..., which means "the property is probably not present at all"

We already have case 3., which is the default behaviour of cds-typer when not specifying --propertiesOptional false.
The main difference between 1. and 3. I am aware of is how they react wrt the in operator et al.:

const o: {
    x: string | undefined,
    y?: string
} = {
    x: undefined
}

console.log('x' in o)  // true
console.log(Object.hasOwn(o, 'x'))  //true
console.log('y' in o)  // false
console.log(Object.hasOwn(o, 'y'))  // false

An absent property and one that is set to undefined usually feel the same, as accessing them via optional chaining and similar constructs will coalesce to undefined.

I believe it is fair to assume that a property in a CDS entity is either absent, which is covered by the default ?:, or that it has been set to nothing, which would be null. Setting a property explicitly to undefined kind of goes against its basic assumption.

I therefore think ... | null makes the most sense. If there is a good reason to add ... | undefined to it, I am absolutely open to that, but starting with a more restrictive type and loosening it later is usually a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request keepalive Will not be closed by Stale bot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants