moved from types to classes

[antmendoza]

Many thanks for submitting your Pull Request :heart:!

What this PR does / why we need it: closes #111 closes #104

Special notes for reviewers:

There are many files changes but we can resume them in:

• updated README.md
• remove types and create classes instead.
• added to builder constant and 'oneOf' related logic
• create constructors on every class that populate default values, special implementation for workflow
• removed WorkflowConverter and moved methods to workflow (fromSource, toYaml, toJson)
• definitions are generated now into src/lib/schema/types/workflow.ts
• format validation error output

I am still working in the generate-classes file to have most of the code autogenerated, will create a PR soon. Additional information (if needed):

[antmendoza]

format validation error output

this is not related with the rest of the changes, I can create another PR for this if you want

[antmendoza]

After reading all your comments @JBBianchi most of then are related to setting default values and constants and I think that everything can be merged in the constructor, as you've mentioned

👍

[antmendoza]

By using the constructor in the builders makes the test fail, since serialized workflow (json file) do not has set default values like type:rest for Function class.

see https://github.com/antmendoza/sdk-typescript/commit/86d45834fca1ec9ccc8f01bb350d9a23d57985ce#diff-c590f689b760b021149371854a8e8dcf45b515f719c7a8abfd5dafea608cf7f2R136

[JBBianchi]

By using the constructor in the builders makes the test fail, since serialized workflow (json file) do not has set default values like type:rest for Function class.

see antmendoza@86d4583#diff-c590f689b760b021149371854a8e8dcf45b515f719c7a8abfd5dafea608cf7f2R136

If you use constructors all the way down, even when deserializing, it should be ok, shouldn't it?

[antmendoza]

deserializing is working ok, serializing is when we are populating default values we don't want to be in the final serialized string

as you mention before, we have to modify the object before serialize it, the place I can see now to do it is in the xxxBuildingFn event if I don't like it.

function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {

    const model = new Specification.Function(data);

    //we want the value only if the client has set it.
    model.type = data.type;

    validate('Function', model);
    return model;
  };
}

having this logic here is weird to me, anyway.

(I am thinking that the proper way might be having two serilize/deserialize methods or classes per type, as they have specific logic )

Let's make this works and we will see later, as the API should not change

I am going to modify the code in this way and see if everything works as expected

[JBBianchi]

Ah yes, sorry, I mixed things up. Serializing is indeed a bit tricky (maybe even more than "a bit")... The objects could maybe have a makeSerializable() method that would recursively delete default values:

export class Workflow {

  makeSerializable() {
    const clone = deepCopy(this);
    // managing self defaults
    if (clone.expressionLang === 'jq') {
      delete clone.expressionLang;
    }
    // deeply removing defaults
    if (typeof clone.start !== typeof '') {
      clone.start = clone.start.makeSerializable();
    }
    if (clone.execTimeout) {
      clone.execTimeout = clone.execTimeout.makeSerializable();
    }
    //...
    clone.states = clone.states.map(s => s.makeSerializable());
    return clone;
  }

  static toJson(workflow: Workflow): string {
    validate('Workflow', workflow);
    return JSON.stringify(workflow.makeSerializable());
  }

  static toYaml(workflow: Workflow): string {
    validate('Workflow', workflow);
    return yaml.dump(workflow.makeSerializable());
  }

}

[antmendoza]

I don't think we should delete always default values, only if the user/client has not set them

[JBBianchi]

How to differentiate the two use cases?

[antmendoza]

We can't, to make it work I am re-setting the value after creating the object, in the builder. Pretty ugly, I know.

function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {
    const model = new Specification.Function(data);

    //HERE
    model.type = data.type;

    validate('Function', model);
    return model;
  };
}

I am testing also the implementation by "hidratating" non-primitive object and I think that it is not enough, since non-primitive properties are created as Object type

    const data = {
      "events": [
        {
          "name": "CarBidEvent",
          "type": "carBidMadeType",
          "source": "carBidEventSource"
        }
      ]
    }

    const model = new Workflow(data);
   // @ts-ignore
   expect(model.events[0]!.constructor.name).toBe("Eventdef"); -> // Expected 'Object' to be 'Eventdef'.

so for each non primitive property I am reseting it by instantiating the corresponding class

[JBBianchi]

We can't, to make it work I am re-setting the value after creating the object, in the builder. Pretty ugly, I know.

function functionBuildingFn(data: Specification.Function): () => Specification.Function {
  return () => {
    const model = new Specification.Function(data);

    //HERE
    model.type = data.type;

    validate('Function', model);
    return model;
  };
}

I don't understand why you'd set type again here, model.type is set inside the constructor ... Am I missing something?

I am testing also the implementation by "hidratating" non-primitive object and I think that it is not enough, since non-primitive properties are created as Object type

    const data = {
      "events": [
        {
          "name": "CarBidEvent",
          "type": "carBidMadeType",
          "source": "carBidEventSource"
        }
      ]
    }

    const model = new Workflow(data);
   // @ts-ignore
   expect(model.events[0]!.constructor.name).toBe("Eventdef"); -> // Expected 'Object' to be 'Eventdef'.

so for each non primitive property I am reseting it by instantiating the corresponding class

In the current state of workflow.ts, I don't see the hydration of events. If the non primitive properties are not instanciated, they indeed are just plain objects. The constructor is missing

if (model.events && typeof model.events !== typeof '') {
  this.events = model.events.map(e => new Eventdef(e));
}

Another concern I just thought about: what if the user sets a property outside the constructor... ?

const wf = new Workflow();
wf.events = [
  {
    name: "CarBidEvent",
    type: "carBidMadeType",
    source: "carBidEventSource"
  }
];

This would assign a plain object to the events property, it would not be hydrated ... :/

The workaround would be to define the properties with getters/setters to hydrate objects instead of doing it in the constructor ...

export class Workflow {
  private _events?: Events;
  get events(): Events {
    return this._events;
  }
  set events(value: Events) {
    if (value && typeof value !== typeof '') {
      this.events = value.map(e => new Eventdef(e));
    }
    else {
      this.events = value;
    }
  }

  constructor(model: any) {
    const defaultModel = { expressionLang: 'jq' } as Specification.Workflow;
    Object.assign(this, defaultModel, model);
  }
}

but that's a lot of hassle...

[antmendoza]

I don't understand why you'd set type again here, model.type is set inside the constructor ...

I have added a test @JBBianchi

hope this helps

[JBBianchi]

I don't understand why you'd set type again here, model.type is set inside the constructor ...

I have added a test @JBBianchi

hope this helps

Sorry, it's Monday morning, maybe I'm not awake but I still don't understand. "In memory" objects should have the default value...

This line of function-builder is redundant:

model.type = data.type;

It's already assigned when calling:

const model = new Specification.Function(data);

As the function constructor already assign the values:

Object.assign(this, defaultModel, model);

In other words:

const fn1 = new Function({ name: 'function', operation: 'operation' }); // fn1.type === 'rest'
const fn2 = functionBuilder().name('function').operation('operation').build(); // fn2.type === 'rest'
const fn3 = new Function({ name: 'function', operation: 'operation', type: 'grpc' }); // fn3.type === 'grpc'
const fn4 = functionBuilder().name('function').operation('operation').type('grpc').build(); // fn4.type === 'grpc'
const fn5 = new Function({ name: 'function', operation: 'operation', type: 'rest' }); // fn5.type === 'rest'
const fn6 = functionBuilder().name('function').operation('operation').type('rest').build(); // fn6.type === 'rest'
fn1.makeSerializable(); // { name: "function", operation: "operation" }
fn2.makeSerializable(); // { name: "function", operation: "operation" }
fn3.makeSerializable(); // { name: "function", operation: "operation", type: 'gprc' }
fn4.makeSerializable(); // { name: "function", operation: "operation", type: 'gprc' }
fn5.makeSerializable(); // { name: "function", operation: "operation" } <-- default is gone even if the user specified it
fn6.makeSerializable(); // { name: "function", operation: "operation" } <-- default is gone even if the user specified it

Whenever the user set the default value or not, the value exists in memory but shouldn't get outputted in the serialized object I think. It's "not possible" to flag whenever the user actually set the property and still output the value in the serialized object if he did. At least, I don't think it's necessary/makes sense.

[antmendoza]

It is Monday for everyone @JBBianchi

I am not sure but I think that if the user set the value, the value should be outputted ,at least that would be the behaviour I was expecting as a client using the library.

As far I have seen, this only is happening with workflow.expressionLang and function.type

[JBBianchi]

I think usedForCompensation might also be concerned, eventstate.exclusive, operationstate.actionmode, parallelstate.completionType, subflowstate.waitForCompletion, workflow.end / workflow.end.compensate, repeat.continueOnError, workflow.keepActive, exectimeout.interrupt, transition.compensate, onevents.actionMode ...

[antmendoza]

👍 Thank you @JBBianchi , I am going to have a look

[antmendoza]

@JBBianchi I have added a couple of test for subflowstateBuilder and eventstateBuilder

https://github.com/serverlessworkflow/sdk-typescript/pull/114/commits/615e174eee073d82e7619a9d0005ff3e6d76f3f4

how do you think this should work ?

[antmendoza]

Ok, sorry I miss all the defaults when get rid of 'class-transformer'

so putting this on code, the behaviour should be something like

describe('eventstateBuilder ', () => {
  it('should build an object', () => {

    const object = eventstateBuilder()
      .name('Book Lending Request')
      .onEvents([oneventsBuilder().eventRefs(['Book Lending Request Event']).build()])
      .transition('Get Book Status')
      .build();

    expect(object.exclusive).toBeTrue();

    const serializedObject = object.serialize();
    expect(serializedObject).toBe(
      JSON.stringify({
        type: 'event',
        name: 'Book Lending Request',
        onEvents: [
          {
            eventRefs: ['Book Lending Request Event'],
          },
        ],
        transition: 'Get Book Status',
      })
    );

    const deserializedObject = object.deserialize(serializedObject)
    expect(deserializedObject.exclusive).toBeTrue();

    //Having serialize/ deserialize de following signature

    //  deserialize(value: string): Eventstate {

    //   }
    //   serialize(): string {

    //   }
    //  

  });
});

[JBBianchi]

I think that's the idea. I don't know if you need a serialize() method on each object but at least a method to get rid of the defaults (in my example, makeSerializable() (which is an ugly name) returns an object. It is required to traverse nested objects to rebuild nested objects without defaults)

[antmendoza]

I think we should move serialize/deserialize functions to another class, workflowSerializer, functionSerializer..., otherwise the compiler is complaining all the time because objects does not have serialize/deserialize properties, which makes sense.

Making them optionals does not seem the solution.

BTW I am not sure that deserialize is needed

[JBBianchi]

otherwise the compiler is complaining all the time because objects does not have serialize/deserialize properties, which makes sense.

I'm not sure to understand what you mean by "the compiler is complaining all the time" but I'm not against having dedicated classes for those.

Note that serialize/deserialize should rather be toJson/toYaml/fromString (or maybe serialize(obj, SerializationFormat.Json) for instance) I think, like we have in the current workflow-converter.

[antmendoza]

Added default values to each constructor and normalize method when required, to delete recursively properties whose value = default value

[antmendoza]

@tsurdilo @JBBianchi this PR is ready to be reviewed.

@JBBianchi since we are creating object for each non-primitive property in the constructor , do you thing that hydration is still needed?

[JBBianchi]

You did an amazing job @antmendoza, congratz! 👏

@tsurdilo @JBBianchi this PR is ready to be reviewed.

I'll try to digg into the details and review asap. I did a quick test and a very fast look around, it seems great. Maybe a detail or two that I'll point but it will probably be more about code style/preference rather than concepts.

@JBBianchi since we are creating object for each non-primitive property in the constructor , do you thing that hydration is still needed?

I don't understand the question, creating a new instance for each non promivite prop is hydration. Instead of having "dumb" objects, you recursively hydrated each object to its instance. So if I'm not missing something, it's all good here !

Thanks again mate, you did great!

[antmendoza]

Hi @JBBianchi @tsurdilo

did you have the chance to look into the PR...?

this is not a final code, there will be more changes in the sdk for sure... If there is no any major issue I would like to have this merge at some point soon in order to cover the 0.6 spec version

If you need any clarification please let me know 🙂

[JBBianchi]

I'm very sorry I didn't have the opportunity to check in depth. At a first glance, it looks good so let's merge, we can still change things if needed.