Persist sort order returned by normalizr #18

coffenbacher · 2018-04-18T18:16:09Z

When data is returned from a REST API in an array
It often has a useful sort order
That should be available to client code.

This data is returned from the normalizr.normalize
Function as result, but currently only entities
Are persisted.

Addresses #17

coffenbacher · 2018-04-18T18:18:46Z

@mikestone14 @pachun can you take a look at this one? I can't request reviewers on this repo

pachun

This looks alright to me. @mikestone14 will have more context. The explanation by Dan Abramov which you liked to from the issue you created was really helpful. I'd like to keep that in the commit message. Otherwise, I think this is good to go pending Mike's approval.

mikestone14

Thanks for contributing @coffenbacher! A couple notes:

This seems to cover the case of the loadAll action in which we would want to replace sortedIds with payload.data.sortedIds in the reducer. I think this is missing the cases for create, load, and destroy.

Scenarios:

create: The resultIds (containing just the newly created entity id) should be added to a new copy of the existing resultIds and replace resultIds.

load: If the loaded entity's id exists in resultIds we should do nothing with that key. If the loaded entity's id does not exist in resultIds we should add it to the end of the resultIds array.

destroy: We should remove the destroyed entity's id from from the resultIds array.

mikestone14 · 2018-04-18T18:46:08Z

package.json

@@ -1,6 +1,6 @@
 {
  "name": "redux-entity-config",
-  "version": "2.0.0",
+  "version": "2.0.1",


This shouldn't be updated as part of the PR but can be done as part of a new tagged commit for version update along with an update to the changelog.

Okay cool, I guess I leave that to you then?

coffenbacher · 2018-04-18T19:19:18Z

@mikestone14 @pachun I added a new version that covers and tests the Create/Load/Destroy scenarios

BlakeWilliams · 2018-04-18T19:23:00Z

I don't think it'd be unreasonable to expect the client to sort the records it retrieves.

For example, storing each returned result by id in an object (for access/performance reasons when working with individual results) you'd want to do something like: Object.values(records) which I don't believe guarantees order.

Not saying we should or shouldn't, just throwing the thought of "maybe the client should be responsible for order" out there.

mikestone14

Looking good @coffenbacher just a couple more comments. I'm also curious why the existing tests are passing if we're changing the shape of the payload in the reducer. I'd be happy to pair on this later if that would be helpful.

mikestone14 · 2018-04-19T13:15:12Z

src/config.js

@@ -55,6 +59,7 @@ class ReduxEntityConfig extends BaseConfig {
            ...state,
            loading: false,
            errors: {},
+            sortedIds: _without(state.sortedIds, payload.data),


Should this be payload.data.sortedIds? This also assumes that the destroyed entity is returned by the server which is not always (maybe even rarely) the case. There is another issue for handling deleting entities though so not in scope for this change.

Yup, I think it should.

For posterity: no change to the sortedIds array for a deletion, since there's ambiguity as to whether an object will be returned with the API response.

mikestone14 · 2018-04-19T13:16:19Z

src/config.js

@@ -45,6 +48,7 @@ class ReduxEntityConfig extends BaseConfig {
            ...state,
            loading: false,
            errors: {},
+            sortedIds: _uniq([...state.sortedIds, ...payload.data.sortedIds]),
            data: {
              ...state.data,
              ...payload.data[entityName],


Should this be payload.data.entities[entityName]?

Based on this documentation of normalizr, I think you're right. The code on L41 would need to be changed too.

But I would expect tests to fail if this code is wrong. Will check out the tests now.

I checked out the test for this code, and it seems to reflect reality. I changed L54 to ...payload.data.entities[entityName] and the test failed. Given those two things, I believe the code is correct as written. I'm not sure exactly where the entities key is removed from payload.data, possibly here.

Turns out, it's happening here when entities is deconstructed.

darkmoves · 2018-05-30T18:53:06Z

Looking into your question about existing tests, @mikestone14, the payload contents are only being tested in config.tests.js via method calls that don't hit the reducer. I wouldn't expect them to fail with this change.

mikestone14 · 2018-05-30T19:51:08Z

@darkmoves you're right but I think that means we're missing test coverage.

darkmoves · 2018-05-31T20:50:41Z

@mikestone14 commit pushed with the changes that we discussed. This should be good to go.

darkmoves · 2018-06-01T17:41:15Z

This PR's been rebased to include the DESTROY fix and the sortedIds array has been added for that action. Ready for another round.

When data is returned from a REST API in an array It often has a useful sort order That should be available to client code. This data is returned from the normalizr.normalize Function as `result`, but currently only `entities` Are persisted. Addresses issue: TheGnarCo#17 Explanation of Normalizr sorting from Dan Abramov: https://github.com/paularmstrong/normalizr/issues/9

pachun requested review from mikestone14 and pachun April 18, 2018 18:19

pachun approved these changes Apr 18, 2018

View reviewed changes

mikestone14 suggested changes Apr 18, 2018

View reviewed changes

mikestone14 reviewed Apr 18, 2018

View reviewed changes

coffenbacher force-pushed the add_normalizr_result_to_success branch 2 times, most recently from c5877a3 to 3f067e4 Compare April 18, 2018 19:13

mikestone14 reviewed Apr 19, 2018

View reviewed changes

darkmoves force-pushed the add_normalizr_result_to_success branch 3 times, most recently from b8dd2e7 to f9ec5c5 Compare June 1, 2018 17:40

mikestone14 approved these changes Jun 1, 2018

View reviewed changes

darkmoves force-pushed the add_normalizr_result_to_success branch from f9ec5c5 to d1b963b Compare June 1, 2018 18:38

mikestone14 merged commit ba37025 into TheGnarCo:master Jun 1, 2018

mikestone14 mentioned this pull request Feb 13, 2019

Server sort order not retained #17

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist sort order returned by normalizr #18

Persist sort order returned by normalizr #18

coffenbacher commented Apr 18, 2018

coffenbacher commented Apr 18, 2018

pachun left a comment

mikestone14 left a comment

mikestone14 Apr 18, 2018

coffenbacher Apr 18, 2018

coffenbacher commented Apr 18, 2018

BlakeWilliams commented Apr 18, 2018

mikestone14 left a comment

mikestone14 Apr 19, 2018

darkmoves May 31, 2018

darkmoves May 31, 2018

mikestone14 Apr 19, 2018

darkmoves May 30, 2018

darkmoves May 31, 2018

darkmoves May 31, 2018 •

edited

Loading

darkmoves commented May 30, 2018

mikestone14 commented May 30, 2018

darkmoves commented May 31, 2018

darkmoves commented Jun 1, 2018

Persist sort order returned by normalizr #18

Persist sort order returned by normalizr #18

Conversation

coffenbacher commented Apr 18, 2018

coffenbacher commented Apr 18, 2018

pachun left a comment

Choose a reason for hiding this comment

mikestone14 left a comment

Choose a reason for hiding this comment

Scenarios:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coffenbacher commented Apr 18, 2018

BlakeWilliams commented Apr 18, 2018

mikestone14 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkmoves May 31, 2018 • edited Loading

Choose a reason for hiding this comment

darkmoves commented May 30, 2018

mikestone14 commented May 30, 2018

darkmoves commented May 31, 2018

darkmoves commented Jun 1, 2018

darkmoves May 31, 2018 •

edited

Loading