Skip to content

Adds afterFind callback and associated specs#1198

Closed
britton-jb wants to merge 1 commit intobalderdashy:masterfrom
britton-jb:feature/afterFind_Callback
Closed

Adds afterFind callback and associated specs#1198
britton-jb wants to merge 1 commit intobalderdashy:masterfrom
britton-jb:feature/afterFind_Callback

Conversation

@britton-jb
Copy link

References issue #923

Adds afterFind callback to the find, findOne, findOrCreate, and findOrCreateEach methods, and callback runner.

Allows users to modify each record that has been found.

Includes specs for running the afterFind callback after find, findOne, and findOrCreate

References issue 923

Adds afterFind callback to the `find`, `findOne`, `findOrCreate`, and
`findOrCreateEach` methods, and callback runner.

Allows users to modify each record that has been found.

Includes specs for running the `afterFind` callback after `find`,
`findOne`, and `findOrCreate`
@devinivy
Copy link
Contributor

Thanks for your contribution! @particlebanana worth taking a peek. I didn't get to read all the tests, but this essentially looks good to me. One curiosity– afterCreate doesn't pass a true model instance but afterFind does. I'm thinking we should fix the discrepancy by adjusting afterCreate rather than the work done in this PR.

@britton-jb
Copy link
Author

@devinivy I can put up a separate PR for the afterCreate model instance fix if needed.

@particlebanana
Copy link
Contributor

For reference this is a extension of #902

@ferrants
Copy link

what's it going to take to get some movement on this PR? I think this should be rebased to incorporate recent changes. Would really like to see this get merged, let me know how I could help.

@mikermcneil
Copy link
Member

@ferrants Thanks for following up! Would you mind putting together a proposal? You can just do it as a comment here, no need to make a separate PR or anything (I'll change the label so it gets surfaced with the others).

Mainly I'm hoping to get an idea of the use case you're after. @particlebanana and I have gone back and forth on this a lot of times and aside from maintenance burden / potential performance hit, there's some conceptual pros and cons to adding these. We did make sure to set things up in WL 0.13 to where implementing arbitrary lifecycle callbacks would be straightforward to add, and consistent (i.e. you can imagine standardized lifecycle callbacks in a future release, where you always just receive a stage 2 query-- meaning you can more easily reuse logic between lifecycle callbacks).

Here's what we were thinking initially. (warning: slightly out of date-- we ended up keeping custom validations, and toJSON support, for example)

Basically where I'm coming from is that, if the times where this is coming up are mostly related to blueprints, then we should handle that outside of waterline. If this is more about being able to coerce something for a JSON response, then we should use toJSON instead. If this is more about being able to coerce records more generally, when using Waterline directly in custom Node.js code, then that's more pertinent here, I'd say.

@mikermcneil
Copy link
Member

@luislobo
Copy link
Contributor

luislobo commented Mar 1, 2017

I've read the proposal (well, the notes about your ideas on how to handle this...), I like it. First I was kind of dubious about removing the before and after validate, but since we can have before and after in other operations, then, validates can be injected there.

@ChrisWorks
Copy link

ChrisWorks commented Aug 23, 2017

Can I ask why the beforeFind and afterFind lifecycle calls are still not done 2-3 years after the features were first requested? The use-cases have not changed; they are still very much valid.

@luislobo
Copy link
Contributor

Hi @ChrisWorks, Waterline has been kind of completely rewritten.
I can see from the code that there are plans to implement this (check the following links)

// FUTURE: This is where the `beforeFind()` lifecycle callback would go

// FUTURE: This is where the `afterFind()` lifecycle callback would go

As with any open source project, things are done by the authors in their available time.
If you see that you can collaborate adding the support for beforeFind and afterFind, I guess the authors will be more than glad to review it. If you do so, please remember to add all the necessary unit tests too.

Best!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

7 participants