cap-js / cds-types

Type definitions for CDS
Apache License 2.0
9 stars 8 forks source link

[BUG] `CDS-types` version 8.0.3 different behaviour on standard events ? #145

Closed dragolea closed 4 weeks ago

dragolea commented 1 month ago

Is there an existing issue for this?

Current Behavior

Hi, In the newly version of the cds-types we have 7 overloads on the AFTER, in the previous version of the CDS 7 we had only 4 overloads.

image

LEFT IMAGE - CDS version 8 RIGHT IMAGE - CDS version 7

During testing a sample project of the standard events, I was not able to find a suitable applied overload for catching the events + the typing looks wrong if we take in consideration also using the CDS-Typer

image

LEFT IMAGE - CDS version 7 RIGHT IMAGE - CDS version 8

I was not able also to trigger the EACH on every Book analogue to the Array.prototype.forEach, practically it doesn't stop.

Expected Behavior

  1. Typing should follow the Entity or should be corrected based on the CDS-Typer entity

References

CDS Version 8.0.3

Versions

This version all overloads are working as expected

@dxfrontier/cds-ts-dis git+https://github.com/dxfrontier/cds-ts-dispatcher.git
@cap-js/asyncapi 1.0.1
@cap-js/cds-typer 0.23.0
@cap-js/openapi 1.0.4
@cap-js/sqlite 1.7.3
@sap/cds 7.9.3
@sap/cds-compiler 4.9.6
@sap/cds-dk (global) 8.0.2
@sap/cds-fiori 1.2.7
@sap/cds-foss 5.0.1
@sap/cds-mtxs 2.0.2
@sap/eslint-plugin-cds 3.0.4
Node.js v22.4.1

This version somehow the overloads are acting awkard

cds8test
@cap-js/asyncapi 1.0.1
@cap-js/cds-typer 0.23.0
@cap-js/cds-types 0.6.0
@cap-js/openapi 1.0.4
@cap-js/sqlite 1.7.3
@sap/cds 8.0.3
@sap/cds-compiler 5.0.6
@sap/cds-dk (global) 8.0.2
@sap/cds-fiori 1.2.7
@sap/cds-foss 5.0.1
@sap/cds-mtxs 2.0.2
@sap/eslint-plugin-cds 3.0.4
Node.js v22.4.1

Anything else? Logs?

No response

hakimio commented 1 month ago

Change from data: Book to data: Books seems to be intentional: https://github.com/cap-js/cds-types/pull/45

dragolea commented 1 month ago

Correct, this is option to have the typing specified inside of the callback But somehow AFTER does not work like before, for EACH and also for normal READ.

daogrady commented 1 month ago

Hi Daniel,

I am not sure I am following. Does the .after('READ', Book, ...) handler not trigger once for each instance of Book in the result of the READ operation?

Best, Daniel

dragolea commented 1 month ago

Hi Daniel

Thanks for taking time to have a look over the issue.

import { Book, Books } from '#cds-models/CatalogService'
import cds from '@sap/cds'

class BooksService extends cds.ApplicationService {
  init() {
    this.after('READ', Books, (data, req) => {
      // data is of type Book and not Books
      debugger
    })

    // This is not triggered anymore like analogue forEach
    this.after('EACH', Books, (data, req) => {
      debugger
    })

    return super.init()
  }
}

module.exports = BooksService

I can create a very small repo example if you are not sure about code above.

stockbal commented 1 month ago

I created PR #151 and a sample project with runtime tests. The PR should fix the current type inference apart from .drafts. If the handler registration should work for drafts the same way then the singular entity should get drafts: typeof Singular and the plural entity drafts: typeof Plural.

dragolea commented 1 month ago

Hi @daogrady, Is above PR all right, above issue is a showstopper for TS projects as AFTER is a core event.

dragolea commented 1 month ago

?

daogrady commented 1 month ago

Hi Daniel,

I have noticed the messages here, but I am am currently buried with other tasks, sorry. I will get to this asap.

Best, Daniel

Mischa1610 commented 1 month ago

We would also be pretty happy to see progress on this topic because we also use the cds-ts-dispatcher package and this is blocking us from updating to CDS8. If we can support testing PR or some fixes just let us know, happy to help.

Here is the issue in cds-ts-dispatcher that documents the progress on "supporting" CDS 8: https://github.com/dxfrontier/cds-ts-dispatcher/issues/85

FYI @mvk-abs

daogrady commented 4 weeks ago

Hi @stockbal @dragolea @Mischa1610 @hakimio ,

the PR looks good to me. Sorry for the delay everyone. and thanks for the contribution, Ludwig! Let's get this merged and released asap.

Best, Daniel