fix: plural drafts #163

[geert-janklaps]

Fixes #163

[daogrady]

Hi @geert-janklaps,

thank you very much for taking on this issue! I have added a changelog entry and test cases for your contribution -- especially the latter are a bit prickly to write.
I will discuss this change with the runtime team later this day and if they don't object we can include this in the next release.
(FYI @johannes-vogel )

Best,
Daniel

[daogrady]

Update on that: we still need to streamline how and where we want to allow the singular/ plural semantic, so I would like to postpone this PR until we have clarity on that.

[geert-janklaps]

Hi @daogrady,

Sounds good to me! Do you have an idea when this could become part of the release / how long you'd postpone?
Currently I need to perform this change in the node modules folder manually for the handler registration to work (and not fail due to typing issues). This also means that for one of our solutions I'm unable to setup CI/CD pipelines for automated build and deployments. (nothing we can't handle, but it would make life easier for sure)

Cheers,
Geert-Jan

[johannes-vogel]

Hi @daogrady,

Sounds good to me! Do you have an idea when this could become part of the release / how long you'd postpone? Currently I need to perform this change in the node modules folder manually for the handler registration to work (and not fail due to typing issues). This also means that for one of our solutions I'm unable to setup CI/CD pipelines for automated build and deployments. (nothing we can't handle, but it would make life easier for sure)

Cheers, Geert-Jan

hi @geert-janklaps,

could you try registering an .after('each', ...) handler instead? See https://cap.cloud.sap/docs/node.js/core-services#srv-after-request

That might avoid fiddling in the node modules.

[github-actions]

This PR has not been updated in a while. If it is still relevant, please comment on it to keep it open. The PR will be closed soon if it remains inactive.

[geert-janklaps]

Hi @daogrady,

Any update on this one by any chance? (mainly commenting to prevent this from being closed automatically)

Cheers,
Geert-Jan

[daogrady]

Hi Geert-Jan,

sorry for the long silence on this one! The blocking issues have been resolved afaik, so I will bring this up again in the next cds-types meeting.

Best,
Daniel

P.S.: bummer I didn't run into you at recap 😕

[daogrady]

Hi Geert-Jan,

I have not forgotten about this, but finding the right solution here is non-trivial. On the one hand, it would be correct to use a plural type for the .drafts on plural. On the other hand, it would mess with how types are derived in handler registrations, so you will probably get Array<Array<Singular>> in some cases. This needs some more thinking through, I believe. Sorry for the wait. :(

Best,
Daniel

[geert-janklaps]

Hi Geert-Jan,

I have not forgotten about this, but finding the right solution here is non-trivial. On the one hand, it would be correct to use a plural type for the .drafts on plural. On the other hand, it would mess with how types are derived in handler registrations, so you will probably get Array<Array<Singular>> in some cases. This needs some more thinking through, I believe. Sorry for the wait. :(

Best, Daniel

Hi @daogrady,

No worries, in this case there are workarounds as suggested by Johannes above. I might be overlooking some cases (I'm looking at this from an implementation point of view), so definitely take your time to go through details so this doesn't create new bugs :)

Cheers,
Geert-Jan

[github-actions]

This PR has not been updated in a while. If it is still relevant, please comment on it to keep it open. The PR will be closed soon if it remains inactive.

[daogrady]

keep alive

[stockbal]

Hi @daogrady,

I enhanced the tests for CQL and service handler registration for plural drafts in my fork of @cap-js/cds-types. See cds-services.ts and cds-ql.ts.

I could find only one issue when registering the handler for after("READ", Foo.drafts, data => {}), but it was actually not because of the plural draft, but the singular one 😅, which infers the data parameter as a single object.
I explained this issue in the test case for cds-services.ts.

Please have a look when you have some time. From what I can see, nothing would stand in the way of making the switch of Plural.drafts from typeof Singular to typeof Plural.

Regards,
Ludwig

[github-actions]

This PR has not been updated in a while. If it is still relevant, please comment on it to keep it open. The PR will be closed soon if it remains inactive.

[daogrady]

Hi Geert-Jan,

sorry for the very belated feedback. .drafts was just something that was continuously displaced by other tasks, so I was not able to give you an appropriate review in due time.
I just noticed that the underlying issue was actually marked as fixed in another PR. I hope your concerns are thereby addressed and cds-typer now produces .draft properties as you'd expect them to behave.

Best,
Daniel