Skip to content

Conversation

handreyrc
Copy link

Issue#194

This PR intoduces interfaces to model definitions allowing to work with plain objects instead of class instances.
This approach makes it easier handling changes in the model and make it compatible with libs such as immer.

The idea behind this implementation is to keep classes under the hood to handle model properties, however, only plain objects are exposed by unmarshalling JSON/YAML or by using the SKD builders. To achieve that, lodash lib was used to convert class instances to plain shallow copies of the original class instances and so preserving properties and methods.

No changes in the API were be introduced by this PR.
Old and deprecated dependencies of the project were updated.
Immer dependency is only being consumed by in the unit tests.

Copy link
Member

@ricardozanini ricardozanini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is with the changes you made to the schema. Why were they necessary? The rest seems pretty straightforward to me.

@handreyrc
Copy link
Author

handreyrc commented Oct 4, 2025

@ricardozanini,

The changes to the schema were made automatically by the script "update-code-base". That was the first thing I did before start working on the project.
It donwloads the schemas, then it recreates definitions (this is an intermediate step, the actual definitions exposed by the sdk are not auto-generated) and builders based on the downloded schemas.

Please, @ricardozanini @JBBianchi, let me know if it is an issue.
If so, I can port my changes on top of the orignal code and run the script recreate the builders only. It's not a big deal.

Thanks a lot!

@handreyrc
Copy link
Author

handreyrc commented Oct 6, 2025

@ricardozanini,
Just adding more information about the schema. The script I mentioned above is downloading the schema from this URL:
https://raw.githubusercontent.com/serverlessworkflow/specification/0.8.x/schema/workflow.json

@ricardozanini
Copy link
Member

@JBBianchi is on PTO until next week; we will hold this PR until then.

Copy link
Member

@JBBianchi JBBianchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m fine with this approach. Just a small note: since normalize() now returns a plain object (e.g. IWorkflow) instead of an instance, runtime checks like instanceof Workflow will no longer work on the returned value. That’s totally expected for this use case, but it’s worth keeping in mind for any downstream logic that might rely on class instances.

@handreyrc
Copy link
Author

I’m fine with this approach. Just a small note: since normalize() now returns a plain object (e.g. IWorkflow) instead of an instance, runtime checks like instanceof Workflow will no longer work on the returned value. That’s totally expected for this use case, but it’s worth keeping in mind for any downstream logic that might rely on class instances.

@JBBianchi,
Thanks a lot for your guidance here. I tried to not add anything new to the SDK, however, if we want to not cause any issues to those relying on classes it is not possible. I changed my approach keeping in mind that those relying on classes shall not be affected at all.

  • I added a new method ''asPlainObject()" to definition classes so no changes are necessary for those relying on classes. For those working with interfaces just call this same method after the build() and it will do the trick.
  • I also added an optional parameter to "fromSource" method in the workflow definition class, so it will work with classes by default, however, by setting the optional parameter "asPlainObject" to true cause the workflow to be deserialized to a plain object.
  • One last change, the "normalize" method now works with classes and plain objects. Basicaly, the original object instance is evaluated to be a class or a plain object and then it will behave accordingly the input type.

That being said, I think we are good for another round.

Thanks reviewing this work!

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 this pull request may close these issues.

3 participants