search

serverlessworkflow / sdk-typescript

Typescript SDK for Serverless Workflow
https://serverlessworkflow.io/
Apache License 2.0
62 stars 16 forks source link

Rename 'BaseWorkflow' #69

Closed JBBianchi closed 3 years ago

JBBianchi commented 3 years ago

What would you like to be added: The name BaseWorkflow is not very clear by itself. It sounds like it's a base class extended by Workflow but it's more a "helper" that parses and serialises a Workflow. I would suggest WorkflowHelper for the moment even if it doesn't sound like the best possible choice.

Why is this needed: For the sake of clarity/readbility/maintainability.

antmendoza commented 3 years ago

BaseWorkflow might not be the most appropriate name, WorkflowHelper doesn't seem appropriate either, as you mention.

Let's see how the sdk evolves, I am pretty sure that we will come up with a better one.

JBBianchi commented 3 years ago

I thought of WorkflowConverter

WorkflowConverter.fromString(definition);
WorkflowConverter.toJson(workflow);
WorkflowConverter.toYaml(workflow);
tsurdilo commented 3 years ago

In sdk-java we just have:

Workflow workflow = Workflow.fromSource(source);

where from and to methods are static

that seems fine to me if possible to do here as well

JBBianchi commented 3 years ago

In sdk-java we just have:

Workflow workflow = Workflow.fromSource(source);

where from and to methods are static

that seems fine to me if possible to do here as well

A type Workflow is already exported by workflow.ts and I'm pretty sure it's generated, probably with json-schema-to-typescript, so it cannot be edited (even if I think @antmendoza is editing it to add './types', which, now that I think of it, sounds like a bad idea as this file is gonna be overriden next time a modification is made to the specification schema, but I digress). As far as I know (which is not very far so I can be wrong), it's not possible to extend a type in TS (meaning to 'externally' add methods/properties to a type). So, in the end, I don't think we can use Workflow.

antmendoza commented 3 years ago

Hi,

Workflow.fromxxx is also my personal preference, or something like new Workflow(new WorkflowFileSource()) / new Workflow(new WorkflowStringSource()), but as @JBBianchi says, Workflow is a type for now. It does not mean that we can not convert it into a class if we wanted.

Regarding to the generated types from json schema, I spend a couple of days (or more) trying to generate models/types automatically but I didn't manage to do it (probably I didn't provide the right config.. idk). Having had it would have saved a lot of time. This is why I am converting things like this https://github.com/serverlessworkflow/sdk-typescript/blob/f0043c51f05121e5dd4dac7ca7a1b398350d155b/src/model/workflow.ts#L366 to a meaningful types. BTW, to have a types.ts file is just a way to "control" which types have been reviewed 🙄

My initial thought was to generate the types automatically and have something like the builder-pattern library to create the builders for us. But as I said before I didn't manage to generate decent types.

I don't think that regenerate the types is a good idea, but to apply to the actual ones the changes coming from the new json schema version.

Anyway, happy to rethink this and open to discuss ideas 😀