cap-js / cds-typer

CDS type generator for JavaScript
Apache License 2.0
29 stars 10 forks source link

[BUG] TypeScript build plugin copies untranspiled JavaScript files to output directory #390

Closed AnsgarLichter closed 1 week ago

AnsgarLichter commented 1 week ago

Is there an existing issue for this?

Nature of Your Project

TypeScript

Current Behavior

The TypeScript build plugin copies the untranspiled model files from @cds-models to the output directory, without any option to customize this behaviour.

For example, the generated TypeScript file looks like this:

export function _BookAspect<TBase extends new (...args: any[]) => object>(Base: TBase) {
  return class Book extends _._managedAspect(Base) {
    declare ID?: __.Key<number>
    declare title?: string | null
    declare descr?: string | null
    declare author?: __.Association.to<Author> | null
    declare author_ID?: __.Key<number> | null
    declare genre?: __.Association.to<Genre> | null
    declare genre_ID?: __.Key<number> | null
    declare stock?: number | null
    declare price?: number | null
    /**
    * Type for an association to Currencies
    * 
    * See https://cap.cloud.sap/docs/cds/common#type-currency
    */
    declare currency?: _.Currency | null
    declare currency_code?: __.Key<string> | null
    declare image?: Buffer | string | {value: import("stream").Readable, $mediaContentType: string, $mediaContentDispositionFilename?: string, $mediaContentDispositionType?: string} | null
    static override readonly kind: "entity" | "type" | "aspect" = 'entity';
    declare static readonly keys: __.KeysOf<Book>;
    declare static readonly elements: __.ElementsOf<Book>;
    static readonly actions: typeof _.managed.actions & Record<never, never>;
  };
}

cds-typer does generate the following js file (for JavaScript projects):

// This is an automatically generated file. Please do not change its contents manually!
const cds = require('@sap/cds')
const csn = cds.entities('sap.capire.bookshop')
// Books
module.exports.Book = { is_singular: true, __proto__: csn.Books }
module.exports.Books = csn.Books
// Authors
module.exports.Author = { is_singular: true, __proto__: csn.Authors }
module.exports.Authors = csn.Authors
// Genres
module.exports.Genre = { is_singular: true, __proto__: csn.Genres }
module.exports.Genres = csn.Genres
// events
// actions
// enums

This does not include the typings when I want to depend on the types, e. g. in the following sample:

import { Book as bookModel } from "#cds-models/sap/capire/bookshop";

export default class Books extends bookModel {

    public greet() {
        console.log(`You are reading ${this.title}`);
    }

}

tsc does not report an error during development. As soon as cds-ts watch is used to execute the project, this leads to the following error: image

This is due to the fact that the generated js file doesn't correspond to the transpilation result, when tsc is executed for this file. Therefore, I changed my tsconfig.json that ts-node transpiles @cds-models' TypeScript files:

{
  "compilerOptions": {
    "rootDirs": [
      "./srv",
      "./@cds-models"
    ],
    "paths": {
      "#cds-models/*": [
        "./@cds-models/*",
        "./@cds-models/*/index.ts"
      ]
    },
    "outDir": "./gen",
    "target": "ESNext",
    "module": "NodeNext",
    "moduleResolution": "NodeNext",
    "skipLibCheck": true,
    "sourceMap": true,
    "allowJs": true,
    "strict": true,
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "resolveJsonModule": true,
    "types": [
      "@cap-js/cds-types"
    ],
    "incremental": false
  },
  "exclude": [
    "./app/**/*",
    "./gen/**/*",
    "eslint.config.mjs",
    "commitlint.config.ts",
    "./@cds-models/**/*.js"
  ],
  "ts-node": {
    "require": [
      "tsconfig-paths/register"
    ],
    "preferTsExts": true
  }
}

For this to work, install npm i -D tsconfig-paths. Now this code works, when cds-ts watch is used. If I know use cds build to build my project for deployment, tsc is also used and copies the transpiled models to the output directory with the tsconfig above. Nevertheless, after tsc is finished the plugin copies the original @cds-model files to the gen folder which then leads to the error shown above when executed:

async build() {
        await this.#runCdsTyper()
        const buildDirCdsModels = path.join(this.task.dest, this.#modelDirectoryName)
        // remove the js files generated by the nodejs buildtask,
        // leaving only json, cds, and other static files
        await rmFiles(this.task.dest, ['.js', '.ts'])

        try {
            await (buildConfigExists()
                ? this.#buildWithConfig()
                : this.#buildWithoutConfig()
            )
        } catch (error) {
            throw error.stdout
                ? new Error(error.stdout)
                : error
        }

        //If I remove the following line it works as expected
        this.#copyCleanModel(buildDirCdsModels)
    }

The workaround is to modify the executed commands during build as following:

build-parameters:
before-all:
  - builder: custom
    commands:
      - npm ci
      - npx cds build --production
      - npx rimraf gen/srv/**/**/*.js
      - npx tsc

Now this works as expected and the transpiled js files from @cds-model are used instead of the untranspiled files. In my example the file gen/@cds-models/sap/capire/bookshop/index.js looks like the following:

"use strict";
var __createBinding = (this && this.__createBinding) || (Object.create ? (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    var desc = Object.getOwnPropertyDescriptor(m, k);
    if (!desc || ("get" in desc ? !m.__esModule : desc.writable || desc.configurable)) {
      desc = { enumerable: true, get: function() { return m[k]; } };
    }
    Object.defineProperty(o, k2, desc);
}) : (function(o, m, k, k2) {
    if (k2 === undefined) k2 = k;
    o[k2] = m[k];
}));
var __setModuleDefault = (this && this.__setModuleDefault) || (Object.create ? (function(o, v) {
    Object.defineProperty(o, "default", { enumerable: true, value: v });
}) : function(o, v) {
    o["default"] = v;
});
var __importStar = (this && this.__importStar) || function (mod) {
    if (mod && mod.__esModule) return mod;
    var result = {};
    if (mod != null) for (var k in mod) if (k !== "default" && Object.prototype.hasOwnProperty.call(mod, k)) __createBinding(result, mod, k);
    __setModuleDefault(result, mod);
    return result;
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.Genres = exports.Genre = exports.Authors = exports.Author = exports.Books = exports.Book = void 0;
exports._BookAspect = _BookAspect;
exports._AuthorAspect = _AuthorAspect;
exports._GenreAspect = _GenreAspect;
// This is an automatically generated file. Please do not change its contents manually!
const _ = __importStar(require("./../../.."));
const __ = __importStar(require("./../../../_"));
const _sap_common = __importStar(require("./../../common"));
function _BookAspect(Base) {
    return class Book extends _._managedAspect(Base) {
        static kind = 'entity';
        static actions;
    };
}
class Book extends _BookAspect(__.Entity) {
}
exports.Book = Book;
Object.defineProperty(Book, 'name', { value: 'sap.capire.bookshop.Books' });
Object.defineProperty(Book, 'is_singular', { value: true });
class Books extends Array {
    $count;
}
exports.Books = Books;
Object.defineProperty(Books, 'name', { value: 'sap.capire.bookshop.Books' });
function _AuthorAspect(Base) {
    return class Author extends _._managedAspect(Base) {
        static kind = 'entity';
        static actions;
    };
}
class Author extends _AuthorAspect(__.Entity) {
}
exports.Author = Author;
Object.defineProperty(Author, 'name', { value: 'sap.capire.bookshop.Authors' });
Object.defineProperty(Author, 'is_singular', { value: true });
class Authors extends Array {
    $count;
}
exports.Authors = Authors;
Object.defineProperty(Authors, 'name', { value: 'sap.capire.bookshop.Authors' });
function _GenreAspect(Base) {
    return class Genre extends _sap_common._CodeListAspect(Base) {
        static kind = 'entity';
        static actions;
    };
}
/** Hierarchically organized Code List for Genres */
class Genre extends _GenreAspect(__.Entity) {
}
exports.Genre = Genre;
Object.defineProperty(Genre, 'name', { value: 'sap.capire.bookshop.Genres' });
Object.defineProperty(Genre, 'is_singular', { value: true });
/** Hierarchically organized Code List for Genres */
class Genres extends Array {
    $count;
}
exports.Genres = Genres;
Object.defineProperty(Genres, 'name', { value: 'sap.capire.bookshop.Genres' });
//# sourceMappingURL=index.js.map

Expected Behavior

Expected behaviour is that tsc is used to transpile the TypeScript files and instead of copying the untranspiled JavaScript files when executed in a TypeScript cds project or at least an option in cds.env.build is given so that this behaviour can be customized. So in my use case the line this.#copyCleanModel(buildDirCdsModels) could be removed in the build plugin and everything would work fine. As I think there is a reason for this line to exist, I file this issue to gather more insights how to mitigate this issue.

Steps To Reproduce

  1. cds init bookshop --add typescript,sample,typer,mta
  2. npm i -D tsconfig-paths
  3. Use the tsconfig.json as shown above
  4. Run cds build --production and vaildate the result in gen/@cds-models/sap/capire/bookshop/index.js

Environment

- **OS**: macOS 15
- **Node**: 22.7.0
- **npm**: 10.9.0
- **cds-typer**: 0.28.1
- **cds**: 8.4.1

Repository Containing a Minimal Reproducible Example

No response

Anything else?

No response

daogrady commented 1 week ago

Hi Ansgar,

thanks for your report! This actually has to be viewed from another perspective: the .ts files are the ones that are wrong. What you see in the .js files is what you will actually get during runtime. There are no classes. We just present these classes on type level to give the user a better feel for their model at design time. But these classes do not actually exist at runtime and can not be shoehorned into cds. They are therefore meant purely as a design time artefact and are not supposed to be treated as actual classes to inherit from.

I guess there is an XY-problem at play here. Why do you need to extend the generated classes?

Best, Daniel

AnsgarLichter commented 1 week ago

Hi Daniel,

thanks for your response - sorry for the wrong classification as bug. I'm extending the generated classes to add methods that are needed for our model because it makes sense from an OOP perspective to include these methods in the classes. The advantage of using those generated classes is that you don't have to manually repeat the property names and adapt them when the corresponding cds model is changed - this happens automatically via cds-typer.

Of course, you have to create an instance after selecting the data manually to be able to use those methods:

const data = await SELECT
      .from(Books);

 const book = new Book(data);
 book.greet();

Alternatively it is also possible to just design the class as follows without extending but the first approach looks better:

import { Book as bookModel } from "#cds-models/sap/capire/bookshop";

export default class Books {

   #data: booksModel

    constructor(data: BooksModel) {
        this.#data = data;
    }

    public greet() {
        console.log(`You are reading ${this.#data.title}`);
    }
}

As the first approach works during runtime, locally and also in a deployed state with the workaround, I thought that this is not a problem of what you get during runtime, rather of the transpilations results. Maybe JavaScript's nature helps to cover some things happening in the background.

Best, Ansgar

daogrady commented 1 week ago

Hi Ansgar,

I'm not an avid cds user myself, so more proficient users can probably provide more insight on this. But this doesn't look like an idiomatic service implementation. I have not seen model-data handling outside of a service's handler function so far, where all of the instances are passed in as parameter of that handler:

class MyService extends cds.ApplicationService { init () {
  this.on('READ', Book, data => { /* use the "instances" in here */ }
}}

The functionality you showed is achievable by passing your entities to an arbitrary function:

function greet (b: Book) {
  console.log(`You are reading ${b.title}`);
}

const book = await SELECT.one.from(Books)
greet(book)

This should always be possible, as all properties in the generated classes are public, so no strict data encapsulation is taking place.

I wouldn't say the cds runtime strictly embraces the OOP paradigm, to be honest. The fact that we chose classes to represent cds artefacts during runtime is purely for convenience[^1]. Entity instances are POJOs, rather than class instances.

Does this help?

Best, Daniel

[^1]: we could have used types instead of classes, but we were facing incompatibility issues with pure JS projects at the time.

AnsgarLichter commented 1 week ago

Hi Daniel,

please excuse the late response. In larger projects it makes sense to distribute your code to keep the code readable and understandable. As there are many ways to organise it, we tried to organise our code like it is done in OOP and then stumbled on this behaviour. Obviously, there are also many other very valid ways to achieve this.

In my opinion, the JS used in the deployed state should correspond to the TypeScript used for development because otherwise you loose some of TypeScript's great benefits. Nevertheless, I understand why the build is developed at it is, thanks to your explanation.

I think we can close this issue. There is a workaround and I don't think we will fully agree on each other's POVs.

Best, Ansgar

daogrady commented 1 week ago

Hi Ansgar,

glad you found the workaround to be at least somewhat feasible for your case!

Just to clarify: cds-typer is purely a design time library layered on top of the cds runtime. As such, there is no way for me to control how cds behaves during runtime (and is therefore not subject to my personal opinion). If you believe there should be a change in how cds behaves, then you need to approach the cds runtime team. cds-typer just follows suit.

Best, Daniel