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

Review patch and remove many for consistency #26

Closed
daffl opened this issue Jan 9, 2016 · 6 comments
Closed

Review patch and remove many for consistency #26

daffl opened this issue Jan 9, 2016 · 6 comments

Comments

@daffl
Copy link
Member

daffl commented Jan 9, 2016

Initially brought up in feathersjs-ecosystem/feathers-sequelize#7, retrieving the records may not behave consistently and also won't work well when pagination is enabled.

@ekryski
Copy link
Member

ekryski commented Jan 11, 2016

Hmm I saw the original issue in feathers-sequelize. Do you have know what the issue is with pagination? We have tests for it and they are passing so it should work...

@daffl
Copy link
Member Author

daffl commented Jan 11, 2016

We don't test remove and patch many with pagination enabled. Basically any method that calls another method may behave inconsistently. An example is that patch calls find to get the list of patched items but what if there are hooks on find? It should really go directly to the database.

@ekryski
Copy link
Member

ekryski commented Jan 11, 2016

Ahhh. Ya ok that makes sense. 💩

Yes, when we converted over to this new style it did feel a bit weird but it was keeping things DRY so I just shrugged and moved on. I guess we'll need to not reuse internal methods and just deal with things directly. Damn...

@daffl
Copy link
Member Author

daffl commented Jan 11, 2016

Shouldn't be too much though. Basically just pulling out the direct filtering logic from .find to re-use it in patch and remove.

@ekryski
Copy link
Member

ekryski commented Jan 11, 2016

Ya I think so too.

@daffl
Copy link
Member Author

daffl commented Mar 2, 2016

This should be fixed via #29

@daffl daffl closed this as completed Mar 2, 2016
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