Open cowboyd opened 6 years ago
Thanks @cowboyd!
Sounds like a candidate to fix before a 1.0 release. I'll open a PR with a failing test and tackle this in the first half of this week.
@cowboyd I was not able to reproduce the issue you're describing. Could you provide more detail?
I've got a branch up with the tests for state.slice
https://github.com/flexyford/impagination/compare/task/slice-out-of-bounds
Since an instance of State
is an array-like object and we're delegating to the Javascript slice
method, I would expect this to truncate. I'm curious about the behavior your seeing.
Weird! We had to put in a workaround in to get what we thought was the proper slice behavior https://github.com/folio-org/ui-eholdings/blob/master/src/components/impagination.js#L20-L35
But maybe we were using it wrong 🤔
Yea from the looks of it, does not seem different than what standard JS slice would be returning.
Using your extended ‘slice’ method as a wrapper to datasetState.slice, i’m curious what the values of ‘start’, ‘end’, ‘datasetState.length’ and ‘result.length’ would be.
Maybe @wwilsman could shed some light here since he did the bulk of the implementation.
Hey @flexyford! Sorry it took me so long to get around to looking at this!
So the problem is actually not that the array is the wrong length, but that the values are undefined.
state.slice(0, 2) //=> [undefined, undefined]
So this is the problem we were facing:
setReadOffset
during scroll so that if we quickly scroll half way down the list, we don't end up with ~200 page requestsslice
ing the stateBecause we are waiting to call setReadOffset
, sometimes the slice
involves records outside of the load horizon (which return undefined
via slice
):
state = new State({ pageSize: 10, readOffset: 20 });
state.slice(9, 11); //=> [undefined, { page, content, ... }]
After some digging, this looks like it might be intended? After calling setReadOffset
with a number within the visible list elements, the slice
correctly receives records with isRequested
, isPending
, etc.
The confusion comes from when we use the stats.totalPages
property. For example, with 400 pages at 25 records per page:
let state = new State({ pageSize: 25, stats: { totalPages: 400 } });
expect(state).to.have.lengthOf(10000);
//=> true
expect(state.slice(5000, 5026)).to.have.lengthOf(25);
//=> true
// this is the desired behavior
expect(state.slice(5000, 5026)[0]).to.deep.equal(state.getRecord(5000));
//=> false
// after we set the read offset, this is true
state = state.setReadOffset(5000);
expect(state.slice(5000, 5026)[0]).to.deep.equal(state.getRecord(5000));
//=> true
Since we are debouncing setReadOffset
, our patch for slice
simply uses getRecord
directly instead of relying on state[5000]
to exist. If there is a solution we could implement, maybe it would be setting up the getters for all records when totalPages
is defined. Otherwise, this might be more of an implementation issue.
The standard behavior for slice is to truncate the slice for any values of
start
andend
that are out of bounds, so for example, no matter how you slice an empty array, you still get the empty array:for the dataset state however, slicing with indices that fall outside the length of the state produces array slots that are all inhabited by
undefined