Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR] Pass the collection object for the middleware hooks that only have plainData (aka preInsert) #788

Closed
rafamel opened this issue Sep 10, 2018 · 6 comments

Comments

@rafamel
Copy link
Contributor

rafamel commented Sep 10, 2018

Sometimes we'll want to manipulate other items in the collection or other collections within a preInsert hook. It would be really handy if we had the collection object. The signature would be: myCollection.preInsert((plainData, collection) => { /* Do stuff */ }). How does it sound?

@pubkey
Copy link
Owner

pubkey commented Sep 10, 2018

Hmm. Might be confusing.
At the part where you define the preInsert-hook, you will always have the collection-reference you could use.
Like

myCollection.preInsert((plainData, collection) => { 
  myCollection.doStuff();
 })

@rafamel
Copy link
Contributor Author

rafamel commented Sep 10, 2018

You are right on this. In fact, the fact that you should have the collection available when setting up the hook is the reason why I didn't propose it at first. The issue is I define the hooks separately and then have some logic to build all the collections with their hooks at once. At some point (read, today) I just forgot about what the common usage is and made the FR.

We could close this as far as I'm concerned, as I'm getting the same behavior via plugin, unless you feel like it's something you'd like to implement to account for that use case.

@pubkey
Copy link
Owner

pubkey commented Sep 10, 2018

Maybe we should bind the this-scope to the collection, so you could do

myCollection.preInsert((plainData, collection) => { 
  this.doStuff();
 })

I see a problem with setting the collection as second parameter because sometime the second will be the RxDocument-instance which might be confusing.

@rafamel
Copy link
Contributor Author

rafamel commented Sep 10, 2018

I would personally go for (plainData, collection) as parameters (as long as it is documented I doubt it would cause much confusion) but I can see your point. Binding this would rule out arrow functions, like the one in the example, but nevertheless it's a reasonable compromise. Are you thinking of having that for all hooks or just preInsert?

@pubkey
Copy link
Owner

pubkey commented Sep 10, 2018

Sorry I meant:

myCollection.preInsert(function(plainData) { 
  this.doStuff();
 })

I would add this to all collection-hooks.

@rafamel
Copy link
Contributor Author

rafamel commented Sep 10, 2018

Sounds like a good plan, then!

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

No branches or pull requests

2 participants