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

Roadmap: RTK v1.3.0 #373

Closed
12 of 18 tasks
markerikson opened this issue Feb 19, 2020 · 17 comments
Closed
12 of 18 tasks

Roadmap: RTK v1.3.0 #373

markerikson opened this issue Feb 19, 2020 · 17 comments
Milestone

Comments

@markerikson
Copy link
Collaborator

markerikson commented Feb 19, 2020

v1.3.0 is shaping up to be a somewhat sizeable set of changes:

  • Adding createAsyncThunk and createEntityAdapter
  • Porting redux-immutable-state-invariant over
  • Dropping support for TS <3.5
  • Possibly dropping or cloning the redux-devtools-extension package to see if we can improve shakeability? (or maybe things can get improved upstream)
  • See if Immer drops a new version that tree-shakes better and we can use

Also, we should totally try to use https://github.com/mweststrate/import-size to help improve our shaking and bundle size.

Sketch of tasks to do:

  • Code
  • Write at least one small-ish example app, both to confirm how the API feels in practice and to act as an example
  • Docs
    • Add usage guides for new APIs
      • createAsyncThunk: data fetching in general, standard Redux patterns, how this simplifies
      • createEntityAdapter: normalization in general, a couple recipes for how you could do normalizing yourself and then use these methods to process (like calling normalizr in an action creator, and having a slice check for action.payload.entities.users and process accordingly)
    • Polish API reference pages
    • Cover new APIs in "Usage with TypeScript" page
    • Consider reworking tutorials to use new APIs?
    • Update README and other docs pages to add mentions of new APIs and revise information (per README, we do now have something for entities, just not relationships...)
    • Separate docs pages for the immutable and serialization middleware
  • Update CRA templates to point to rtk@^1.3.0

Ongoing v1.3.0 integration work is in this PR:

v1.3.0 integration

@phryneas
Copy link
Member

phryneas commented Feb 19, 2020

Additionally:

  • give the entity adapter typings a thorough review (I just skimmed them so far)

@chybisov
Copy link

@markerikson how about using keys instead of ids? We already using entity and entities, so I think we should follow common datastore Entities, Properties, and Keys naming. What do you think?

@markerikson
Copy link
Collaborator Author

@chybisov : I'm not sure what you mean by "common naming".

The @ngrx/entity API already has state.entities and state.ids, which I like better than the docs page I wrote suggesting state.byId and state.allids:

https://redux.js.org/recipes/structuring-reducers/normalizing-state-shape

So no, I don't see any reason to change that.

@chybisov
Copy link

chybisov commented Feb 21, 2020

@markerikson I mean standard way to name such things. In database world we have entities and keys to this entities. Pretty straightforward names. Key is more generic name, than id, it would be better.

btw @ngrx/entity already uses keys as variable name in a lot of place, I don't know why they not change the main one. I think most people just don't care :) but for consistency it would be just great.

@markerikson
Copy link
Collaborator Author

I feel like id is more relevant, given that we're also assuming the values have entity.id to begin with. Not planning to change that.

@markerikson
Copy link
Collaborator Author

I'm inclined to punt on the new Immer version for now. Whenever it comes out, it's probably worth its own separate update, probably 1.4.0. Removing that from the list.

@markerikson
Copy link
Collaborator Author

Throwing this here for now because I don't want to open another issue atm. Someone on Reddit showed some interesting notional syntax for generating async action types inside of createSlice:

https://www.reddit.com/r/reactjs/comments/f7ue0y/getting_started_with_the_official_redux_template/fiibthh/

https://i.imgur.com/kcJUcxd.png

const someSlice = createSlice({
    name: "slice",
    initialState,
    reducers: {
        fetchItems: {
            success(state, action) {}
        }
    }
})

Not sold on it, but it does go along with us already having an object notation for prepare callbacks. Worth noting.

@Brodzko
Copy link

Brodzko commented Mar 2, 2020

Not sold on it, but it does go along with us already having an object notation for prepare callbacks. Worth noting.

I'd +1 this suggestion - personally using sagas and no thunks and I ended up wrapping createAction so that the returned action creator returned plain "starting" action and had the creators equivalent to async stages as properties. Essentially ended up with creators like fetch(), fetch.pending(), fetch.resolved(), fetch.rejected(), fetch.cancel().

I quite like how it feels in development, but my API is a tad bit awkward, not to mention I had to do the same thing with reducers (yeah, wrapping createSlice isn't easiest).

I'm pretty confident it could be done though, and having this as a core feature of RTK would be awesome for those of us who use sagas, observables or similar, instead of thunks :)

@CosmaTrix
Copy link

I love the entityAdapter! ❤️

I think that a couple more selectors could be added to it though. Namely selectOne and selectMany. It'd better map with the CRUD operations (where you have addOne, addMany, removeOne, etc...). Moreover, I think it would be useful in overall applications where you have a page listing all the entities and a details page displaying one entity.

What do you think?

@chybisov
Copy link

@CosmaTrix @markerikson it would be very useful, especially selecting some object by id or some other property.

@markerikson
Copy link
Collaborator Author

Yeah, I'd definitely been thinking about adding a selectById. Pretty simple to add. Not sure what selectMany would look like, API-wise. Would it take a list of IDs as a selector param?

@CosmaTrix
Copy link

@markerikson Yup, that's my idea.

// types
selectOne: (state: V, id: EntityId) => T | undefined
selectMany: (state: V, ids: EntityId[]) => Dictionary<T>

// examples
selectors.selectOne(state, 1);
selecttors.selectMany(state, [1, 2, 5]);

I've cloned the repo today and played a little bit with the source code. I've added, typed and tested selectOne and selectMany. The only thing left to do, I guess, would be to update the documentation and then create a PR. Shall I go for it?

@markerikson
Copy link
Collaborator Author

I wouldn't expect selectMany to return a lookup table, but rather an array.

My main question for this is the typical memoization issue. Notionally, an implementation of selectMany would look like:

const selectMany = createSelector(
  selectEntities,
  (state, ids) => ids,
  (entities, ids) => ids.map(id => entities[id])
)

The problem is that a typical usage pattern like selectMany(state, [1, 2, 5]) would pass in a new ids array every time, which would cause the output selector to always recalculate, which would always return a new array reference as a result. Same thing if it's getting used in multiple places to with varying inputs.

(This has always been the weakest aspect of Reselect. Since it only caches the latest calculated value, changing the inputs causes recalculations, which can both be expensive in terms of transformation cost, and returning new references as a result typically causes re-renders at the UI level.)

The current list of generated selectors doesn't have to worry about that, because the only inputs are the EntityState object. Even selectById is okay, because it's a straight lookup. Calling it multiple times with different IDs will technically cache-bust, but it's okay because there's no transformation being done.

Not saying we can't add it, but it's a usage nuance that gets tricky.

@markerikson
Copy link
Collaborator Author

Tell you what. I'll go ahead and add selectById to the next beta, and we can talk about whether it's worth adding some kind of selectMany from there.

@CosmaTrix
Copy link

@markerikson You're so right! Both about the return type of my selectMany (it should in fact be selectMany: (state: V, ids: EntityId[]) => T[]) and about its memoization "issue" with an array being always a different input.
But glad to hear you're going to add the selectById selector. 👍

@markerikson
Copy link
Collaborator Author

Yeah, just put that out in https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0-beta.1 .

@markerikson
Copy link
Collaborator Author

v1.3.0 is now LIVE! https://github.com/reduxjs/redux-toolkit/releases/tag/v1.3.0

We don't have the usage guide docs for createEntityAdapter in yet, but I'm going to close this and add separate issues for the docs work.

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

No branches or pull requests

5 participants