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

Selector API improvements #257

Merged
merged 23 commits into from
May 11, 2019
Merged

Conversation

haveyaseen
Copy link
Member

@haveyaseen haveyaseen commented May 7, 2019

Implements #251 and contains checks to avoid #256.

  • Require passing at least one selector with an ORM reference.
  • Allow and require passing a stateSelector option during ORM construction.
  • Add re-reselect to dependencies.

const publishers = createSelector(orm.Publisher);
  • publishers(state):
Publisher.all.toRefArray()
  • publishers(state, 1): null or
Publisher.withId(1).ref
  • publishers(state, [1, 2]):
[1, 2].map(id => Publisher.withId(id).toRefArray())

const publisherMovies = createSelector(orm.Publisher.movies);
  • publisherMovies(state):
Publisher.all().toModelArray().map(publisher => publisher.movies.toRefArray())
  • publisherMovies(state, 1): [] or
Publisher.withId(1).movies.toRefArray()

// given the following:
const movieRating = createSelector(
    orm.Movie,
    movie => movie.rating
);
// then
const publisherMovieRatings = createSelector(
    orm.Publisher.movies.map(movieRating)
);
  • publisherMovieRatings(state):
createSelector(
    state => state,
    orm.Publisher.movies,
    (state, movies) => movies.map((state, i) => movieRating(state, movies[i].ref.id))
)
  • publisherMovieRatings(state, 1): same as above
  • publisherMovieRatings(state, [1, 2]): same as above
  • Allow for all types of relationship fields (oneToOne, fk, many) only collections
  • Allow passing selector specs as the selector in orm.<Model>.<field>.map(<selector>), e.g:
const authorPublishers = createSelector(
    orm.Author.books.map(orm.Book.publisher)
);
authorPublishers(state, 1);
// Author.withId(1).books.toModelArray().map(book => book.publisher && book.publisher.ref)

  • Remove ORM#createReducer and ORM#createSelector to prevent reselect from being bundled unnecessarily.
  • Cache selectors based on their position in orm.<subtree>:
    • orm.<Model>
    • orm.<Model>.<field>
    • orm.<Model>.<field1>.….<fieldN> recursively
      • only for field 1 to field n - 1 (potentially also field n) being single referenced relationship fields,
        for instance orm.Movie.director.userAccount.paymentAccount.payments
    • orm.<Model>.map(<selector>) same as passing selector
    • orm.<Model>.<field1>.….<fieldN>.map(<selector>)
      • Use nested maps in selector cache so that we can cache by selector references.

To do in the future:

  • Allow passing array of IDs to Model.filter, as in Publisher.filter({ id: [1, 2] }).

@haveyaseen haveyaseen added the status: WIP Someone is working on this issue. label May 7, 2019
@haveyaseen haveyaseen marked this pull request as ready for review May 9, 2019 12:05
@codecov-io
Copy link

codecov-io commented May 9, 2019

Codecov Report

Merging #257 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   99.77%   99.81%   +0.03%     
==========================================
  Files          13       14       +1     
  Lines         909     1074     +165     
  Branches      155      209      +54     
==========================================
+ Hits          907     1072     +165     
  Misses          2        2
Impacted Files Coverage Δ
src/fields.js 100% <ø> (ø) ⬆️
src/memoize.js 100% <100%> (ø) ⬆️
src/ORM.js 100% <100%> (ø) ⬆️
src/selectors.js 100% <100%> (ø)
src/redux.js 100% <100%> (ø) ⬆️
src/db/Database.js 100% <100%> (ø) ⬆️
src/constants.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6077db...de0994a. Read the comment docs.

@haveyaseen haveyaseen added the status: Please Review Waiting for input by contributors. label May 9, 2019
@haveyaseen haveyaseen removed the status: Please Review Waiting for input by contributors. label May 11, 2019
@haveyaseen haveyaseen added type: Enhancement This intends to enhance the project. and removed status: WIP Someone is working on this issue. labels May 11, 2019
@haveyaseen haveyaseen merged commit bcbbb15 into redux-orm:master May 11, 2019
@haveyaseen haveyaseen deleted the better_selector_api branch May 11, 2019 16:36
@ernsheong
Copy link
Collaborator

Is there a migration guide for this?

@haveyaseen
Copy link
Member Author

haveyaseen commented Jun 2, 2019

Not yet, but it's mostly a backwards-compatible change. All you need to do is specify a stateSelector when constructing your ORM and stop passing it to createSelector. This is described in the release notes for v0.14.0-rc.0.

Converting complex selectors should turn out to be quite intuitive. I might write this up a bit in the future, but this will take a while.

@haveyaseen
Copy link
Member Author

haveyaseen commented Jun 2, 2019

One simple example from our tests:

-const movieRating = createSelector(
-    orm,
-    state => state.orm,
-    (state, id) => id,
-    ({ Movie }, id) => Movie.idExists(id) ? Movie.withId(id).rating : null,
-);
+const movieRating = createSelector(orm.Movie.rating);

-const publisherAverageRating = createSelector(
-    orm,
-    state => state.orm,
-    state => state,
-    (state, id) => id,
-    ({ Publisher }, state, id) => {
-        if (!Publisher.idExists(id)) return null;
-        const ratings = Publisher.withId(id).movies.toRefArray()
-            .map(movie => movieRating(state, movie.id));
-        return ratings && (ratings.length ? avg(ratings) : 'no movies');
-    },
-);
+const publisherAverageRating = createSelector(
+    orm.Publisher.movies.map(movieRating),
+    ratings => ratings && (ratings.length ? avg(ratings) : 'no movies'),
+);

publisherAverageRating(store.getState(), 1);

+// now works with arrays as the 2nd argument (for publishers with ID 1, 2, 3 or 4)
+publisherAverageRating(store.getState(), [1, 2, 3, 4]);

+// now also works with nothing as the 2nd argument (for all publishers)
+publisherAverageRating(store.getState());

Note that in the above example there would have been no need to have a separate movieRating selector. .map(movie => movieRating(state, movie.id)) could of course be replaced by .map(movie => movie.rating). But it would have been necessary if movie ratings had been stored per user or something. Imagine the following:

-const movieRating = createSelector(
-    orm,
-    state => state.orm,
-    (state, id) => id,
-    ({ Movie }, id) => {
-        if (!Movie.idExists(id)) return null;
-        const ratings = Movie.withId(id).userRatings.toRefArray();
-        return ratings && (ratings.length ? avg(ratings) : null);
-    },
-);
+const movieRating = createSelector(
+    orm.Movie.userRatings,
+    ratings => ratings.length ? avg(ratings) : null,
+);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
layer: Selectors type: Enhancement This intends to enhance the project.
Projects
Version 1.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants