adonisjs / attachment-lite

Turn any field on your Lucid models to an attachment data type
MIT License
68 stars 20 forks source link

Model `save` function does not return the model anymore #26

Closed aa-ndrej closed 1 year ago

aa-ndrej commented 1 year ago

When a model contains the attachment decorator the save function of the model internally calls the saveWithAttachments function of the decorator and returns undefined instead of Promise<Model>.

Package version

v1.0.7

Node.js and npm version

Node.js: v18.15.0 npm: v9.5.0

Sample Code (to reproduce the issue)

Add the attachment decorator to a model.

hbatalhaStch commented 1 year ago

@thetutlage I am facing the same issue? Any fix or workaround?

aa-ndrej commented 1 year ago

@hbatalhaStch the workaround would be to just treat the save function as if it is not returning anything.

So instead of chaining model.save().otherMethod(); use model.save(); model.otherMethod();. Or instead of returning return model.save() use model.save(); return model;

hbatalhaStch commented 1 year ago

@aa-ndrej thanks for the tip.

I have a question though: I am using the return of model.save() to check if save was successful, is this a good way? Is there a need to check if save was successful?

aa-ndrej commented 1 year ago

@hbatalhaStch you can view the source code of the models save function if you navigate to node_modules/@adonisjs/lucid/build/src/Orm/BaseModel/index.js. I do not have that much experience with adonisjs but as far as I can see the function just returns return this;, so it is not really indicating that the model was successfully saved. But I would assume that the save function would throw some kind of error otherwise.

hbatalhaStch commented 1 year ago

@aa-ndrej

you can view the source code of the models save function...

I already cloned the lucid repo to study to see if it throws somewhere

but as far as I can see the function just returns return this;

I am trying to see if any of the functions called above throws an error @thetutlage would be helpful here.

Thanks

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

RomainLanz commented 1 year ago

Closing in favor of the PR.

RomainLanz commented 1 year ago

Released as 1.0.8