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

Enhance paramsForServer with parameters with specific values. #607

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Yavorss
Copy link

@Yavorss Yavorss commented Jul 2, 2021

Summary

This a solution to the issue with -1 from feathers-hooks-common mentioned here #590. The solution is simple. Instead of filtering only by parameter, we could filter by parameter with a specific value. For instance, paramsForServer: [['$limit', '-1']] or paramsForServer: [['$limit', value => value === '-1']]. This is not a breaking change.

Other Information

If that looks good for the team, we could update the documentation and link that PR to here as well.

@Yavorss Yavorss changed the title paramsForServer as key plus value or result of a function Enhance paramsForServer with parameters with specific values. Jul 2, 2021
Yavor Stoychev added 2 commits July 2, 2021 14:42
@J3m5
Copy link
Contributor

J3m5 commented Jul 2, 2021

Please, take a look at my comment there

@joshuaja
Copy link

joshuaja commented Jul 2, 2021

@J3m5 Allowing a filtering function within theparamsForServer solves the $limit: -1 issue and also adds some needed functionality to the paramsForServer filter.

That would be a more straightforward solution over hard-coding the -1 in multiple places within the code and enhances the utility of paramsForServer at the same time.

@J3m5
Copy link
Contributor

J3m5 commented Jul 2, 2021

Ok I get it.

There is one small problem however.
This MR will conflict with a previous one

And I think the added code could be isolated in a separate function.

@marshallswain
Copy link
Member

This is a good idea. Can you double check the conflicts from the other PRs merged?

@Yavorss
Copy link
Author

Yavorss commented Jul 6, 2021

This is a good idea. Can you double check the conflicts from the other PRs merged?

Will update it these days or even today.

@fratzinger
Copy link
Collaborator

Pretty nice! I also thought about this before.

A mention about this in the docs would be pretty neat! @Yavorss could you add this to the docs?

@marshallswain
Copy link
Member

@Yavorss were you able to make any additional progress on this. I should have some time soon to take a look, if not.

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.

5 participants