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

Proposal to remove JavaScript classes and just generate interfaces #285

Open
davidjgoss opened this issue Feb 6, 2025 · 3 comments · May be fixed by #287
Open

Proposal to remove JavaScript classes and just generate interfaces #285

davidjgoss opened this issue Feb 6, 2025 · 3 comments · May be fixed by #287

Comments

@davidjgoss
Copy link
Contributor

davidjgoss commented Feb 6, 2025

Currently the JavaScript code generated for messages represents each message object as a class, with implicitly public fields and an empty constructor. This requires two dependencies reflect-metadata and class-transformer to make deserialisation work with the parseEnvelope function.

The classes have no methods, and the only thing they do vs just having plain objects is to default missing non-optional properties to '' (in case of strings) or new objects (for classes) which I think is a dubious practise anyway.

I'd propose we just generate TypeScript types/interfaces rather than classes, meaning the schema would carry no code overhead, and no dependencies, and (de)serialisation could just be handled with standard JSON.parse and JSON.stringify as JavaScript users are well accustomed to. As a data point, cucumber-js and our first-party formatters only work with messages as plain objects and never instantiates the classes.

The automatic empty string/object behaviour would go away - it's unclear if anyone is relying on this.

cc @vitalets @badeball as other consumers of this library - any thoughts?

@vitalets
Copy link

vitalets commented Feb 7, 2025

I like it! In most cases I use cucumber/messages for types.
The only question is about enums - are you going to transform this to type as well?

export declare enum TestStepResultStatus {
    UNKNOWN = "UNKNOWN",
    PASSED = "PASSED",
    SKIPPED = "SKIPPED",
    PENDING = "PENDING",
    UNDEFINED = "UNDEFINED",
    AMBIGUOUS = "AMBIGUOUS",
    FAILED = "FAILED"
}

@davidjgoss
Copy link
Contributor Author

Ah right, the enums would stay as-is and still generate some code. (I personally tend to favour a string union for enum use cases, but I wouldn't want to change these ones now.)

@badeball
Copy link
Member

badeball commented Feb 7, 2025

which I think is a dubious practise anyway.

I agree and fully support this, as I doubt it will have any unintended consequences for myself. I'd much rather like to rely on the type system for ensuring non-null values (eg. using ?? operator) instead of built-in defaulting, but that's me.

The messages library also tends to "linger" in the dependency tree (JS), with old versions and deprecation notices, causing some noise, so I'm happy to see it go away.

@davidjgoss davidjgoss linked a pull request Feb 7, 2025 that will close this issue
7 tasks
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 a pull request may close this issue.

3 participants