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

Add support for the disablePagination hook in feathers-hooks-common #590

Open
joshuaja opened this issue Apr 7, 2021 · 5 comments
Open

Comments

@joshuaja
Copy link

joshuaja commented Apr 7, 2021

Steps to reproduce

feathers-hooks-common has a disablePagination hook that allows us to pass in $limit: -1 to disable pagination for a Feathers service.

When passing $limit: -1 to useFind params in feathers-vuex, we have to use both params and fetchParams as a workaround since feathers-vuex doesn't see $limit: -1 as turning off pagination.

Expected behavior

If $limit: -1 is passed to useFind params I would expect for it to add paginate: false to the params and $limit: -1 to the fetchParams in the background.

Actual behavior

feathers-vuex doesn't turn off pagination if $limit: -1 is passed to params.

System configuration

Module versions (especially the part that's not working):

  • "feathers-vuex": "^3.15.0"
  • "vuex": "^3.6.2"

NodeJS version:

  • "node": "14.4.0"
  • "npm": ">= 6.14.5"

Operating System:

  • Linux/Docker

Browser Version:

  • Latest Chrome

Module Loader:

  • Vue CLI
@fratzinger
Copy link
Collaborator

That's something I have to do quite often. FeathersVuex takes $limit: -1 as $limit: 1 and just returns one item.
@marshallswain: is this something that can work natively without any breaking changes for anyone not working with disablePagination? So should this just work by default or should it be an option (e.g.: addSupportForLimitMinusOne)?

As I use it quite often, I would like to add this feature.

@J3m5
Copy link
Contributor

J3m5 commented Jul 2, 2021

To me, it seems that we just need to handle the -1 value specifically in the find getter.
Currently, it is just passed as the last parameter of the slice method there.
We could detect the value and act accordingly by not slicing the items and returning just the items, not the paginated result.

@Yavorss
Copy link

Yavorss commented Jul 2, 2021

To me, it seems that we just need to handle the -1 value specifically in the find getter.
Currently, it is just passed as the last parameter of the slice method there.
We could detect the value and act accordingly by not slicing the items and returning just the items, not the paginated result.

We also discussed that approach. The problem is the following. feathers-vuex uses the function filter-query from feathers and that function transforms the incoming number to absolute value, so -1 turns to 1. That means that we need to implement that logic specifically for the value -1 otherwise that would be a breaking change if we do not transform numbers to an absolute value(it's possible for some projects to rely on this). The discussion of whether -1 should be fixed in the find getter is bigger. #607 solve the problem as a workaround and adds more capability for other cases.

@J3m5
Copy link
Contributor

J3m5 commented Jul 2, 2021

The filter-query behavior doesn't prevent us to do what I suggested.

The filter-query method is called there in the getter,
so we can still check the value of the $limit param before it is called or even after by getting it in the params because the filter-query method doesn't seems to mutate the params.

You can take a look at what id done in the adapter-commons and in the memory adapter.

And making a breaking change is not necessarily a bad thing if the behavior was buggy and if it fix it.
Users relying on a undocumented and buggy behavior shouldn't prevent us from fixing a bug.

@marshallswain
Copy link
Member

We should definitely support this directly. I'm a bit swamped at the moment, so I can't participate much for another like 2 months. If any of you would like to create a PR, I'll make time to review and merge.

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

No branches or pull requests

5 participants