Platform: Consider making ngrx/entity framework-agnostic

[markerikson]

Problem

The @ngrx/entity package provides some useful utilities for dealing with normalized state.

Looking through the source files, most of the logic is actually not specific to NgRX. The type definitions and such seem very reusable, and it's almost completely framework-agnostic (as confirmed by @MikeRyanDev at https://twitter.com/MikeRyanDev/status/1216820265658789888 ).

The only framework-specific imports I can see are uses of import { createSelector } from '@ngrx/store' in a few files, and import { isDevMode } from '@angular/core'; in utils.ts.

I'd be interested in reusing the logic from @ngrx/entity in Redux Toolkit, but in order for that to be possible, the Angular/NgRX-specific imports would need to be removed from this library.

What is the feasibility of that happening?

We could always copy-paste the relevant code over to Redux Toolkit with appropriate citations, but it'd be nice to not have to reinvent the wheel over a couple of import statements.

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[edbzn]

Here is a similar discussion about this idea ( https://github.com/ngrx/platform/issues/2283 ).

[evgeniyefimov]

We are using @ngrx/entity with ngxs, works great but it requires @ngrx/store as a dependency in package.json and we don't use @ngrx/store anywhere.

[brandonroberts]

Hey @markerikson, I think we might be able reasonably support that. What did you have in mind as far as changes to make that work?

[markerikson]

I'm not entirely sure what the desired implementation should actually look like.

As mentioned, the only real framework-specific references here are the imports of createSelector and isDevMode.

It looks like the import of createSelector in create_adapter.ts isn't even being used. It _is_ being used in state_selectors.ts, which is understandable.

Perhaps createAdapter could take a createSelector function as an option, allowing you to either pass in the version from Reselect or @ngrx/store`? I assume (or at least hope) that the two libraries are API/types-compatible enough that you could use either of them.

For the isDevMode usage, it's just warning if a selector returned undefined. The actual implementation of isDevMode is over in the Angular core in is_dev_mode.ts, where it looks like it's controlled by the rest of the build pipeline somehow.

Not sure what a suitable alternative would be here - a process.env.NODE_ENV check? Something else?

[markerikson]

FWIW, I'm playing around with copy-pasting the @ngrx/entity logic over into Redux Toolkit directly, and modifying it based on our own needs.

The first tweaks were just replacing the Angular imports I've mentioned. I'm now looking at the update logic and seeing if there's ways to simplify it using Immer. (I've already updated state_adapter.ts to skip the preemptive copying before running mutator(), and am now looking at the sorted/unsorted adapter files.)

[brandonroberts]

Cool. Let us know how that goes. We'd still like to make this work and share some core code if possible.

[markerikson]

Sure. My first impression is that use of Immer would allow us to drop all the DidMutate logic entirely, which would be a fairly drastic change in internal implementation.

[alex-okrushko]

@markerikson WFIW, adding multiple deps freaks people out. e.g. Angular has the only dep right now, which is rxjs... and so does ngrx. Adding immer won't be something that is taken lightly.

I'll all for reusable lib and would really like to see @ngrx/entity being able to be used in redux, or any other state management 👍, but adding deps to entity itself could be problematic.

[markerikson]

Right, there's multiple thoughts here.

One is that if @ngrx/entity is generic and reusable, it might just be usable as-is.

Another is that as I just found out, the logic in @ngrx/entity can be simplified considerably by using Immer instead of all the manual "did we mutate?" bookkeeping. That's not an issue for RTK, which already heavily depends on Immer, but may not be acceptable for NgRx, which doesn't. In that case, perhaps a straight fork-and-modify for RTK's usage would be more appropriate.

On that note, I just posted a comment over in the RTK issues about my initial attempts to fork-and-modify @ngrx/entity using Immer, including a link to the branch with my changes if anyone would like to look:

https://github.com/reduxjs/redux-toolkit/issues/333#issuecomment-583953456

[MikeRyanDev]

@markerikson I want to give you a lengthier response later and help make sharing @ngrx/entity a success. One thing I want to say now is that if you do pull in @ngrx/entity logic I'd shy away from refactoring the implementation details. One of the earliest design decisions I made with the library is that @ngrx/entity should be extremely fast under the hood. That's why the implementation is so bizarre looking. I never wanted someone opening an issue here blaming perf on @ngrx/entity. 😄

[markerikson]

Sure. Nothing I'm doing so far is final - I'm just playing around with things atm.

I _am_ curious about:

• the perf characteristics of always copying both entities and ids up front
• the perf impacts of using Immer
• the completely separate code paths for sorted vs unsorted collections
• what the net bundle size changes would be for RTK based on the Immer-ified version I was playing around with

[MikeRyanDev]

@markerikson

the perf characteristics of always copying both entities and ids up front

GC pauses for managing ids and entities in an immutable way becomes unbearably slow when performing large upserts or patches to the collections.

the completely separate code paths for sorted vs unsorted collections

The implementations have always differed just enough that they had to be separate. @ngrx/entity sorts a collection at time of modification rather than in a selector. I've always had the itch to take this a step further and swap the sorted collection's implementation to use a heap and then sort the heap in a selector.

Let me know what you find out with regards to Immer's perfomance here. I'm interested as well.

[MikeRyanDev]

Oh, another thing about the sorted collection implementation being different: IIRC when I created @ngrx/entity array.sort() wasn't guaranteed to be a stable sort across browsers. Sorting the collection by hand let me guarantee resorting the collection was stable. This felt important to me from a UX perspective if you built a UI that rendered the collection as a list.

[markerikson]

The Immer docs on overall perf are here:

https://immerjs.github.io/immer/docs/performance

Part of my observation is that while Immer _will_ have additional overhead in comparison to doing things at the "raw JS" level (snicker), there are benefits in that:

• We don't have to pre-copy ids and entities up front, because they'll only get copied at the end by Immer _if_ the update logic actually tried to mutate one of them
• There's savings in code size, because all the logic for trying to track DidMutate just went away completely

No idea what actual benchmarks on this might look like.

And yeah, I was looking at the sorting logic as I was reworking things, because my naive implementation _wasn't_ stable. I noted that there's a test that explicitly reuses the same title twice for a Book, and that one was failing for me. (I actually edited the test to choose a different title on the grounds that I wasn't sure I agreed with the apparent premise in the test, but I understand your point here completely.)

[markerikson]

One other API design thought:

Right now, the EntityAdapter API methods all seem to take an item first, and a state value second. From the usage example in the docs:

  on(UserActions.updateUsers, (state, { users }) => {
    return adapter.updateMany(users, state);
  }),
  on(UserActions.mapUsers, (state, { entityMap }) => {
    return adapter.map(entityMap, state);
  }),

It would be _really_ nice if the adapter arguments were flipped, so that the adapter methods could be used as reducers directly. Similarly, it would be great if the outer adapter wrapper function could check to see if the second argument was a Flux Standard Action with an action.payload field, and default to passing that downwards to the mutator function if so.

That would enable usage like:

const usersAdapter = createEntityAdapter<User>();

const usersSlice = createSlice({
    name: "users",
    state: usersAdapter.getInitialState(),
    reducers: {
        userAdded: usersAdapter.addOne,
        userRemoved: usersAdapter.removeOne,
        // etc
    }
});

[markerikson]

This evening's experimentation:

• Swapped the argument order for stateOperator so that it's now (state, arg)
• Added logic to check if arg is actually a Flux Standard Action, and if so, call mutator(arg.payload, state) instead
• Verified that I could now use usersAdapter.addOne as a Redux reducer directly, by passing it to createSlice

I did run into some weird behavior with Immer, where nested produce calls didn't produce the results I expected:

https://github.com/immerjs/immer/issues/533

That of course wouldn't be an issue if I hadn't just gone in and Immer-ified the guts of the entity logic.

Other than that Immer issue, I _really_ like where this is going. Problem is, I've just forked the original @ngrx/entity API completely by swapping the arguments. As much as I'd like to stay on a shared codebase, it's starting to feel like what I want for RTK would likely be too much of a change to force on NgRx.

@MikeRyanDev : back to the perf thing. Did you have any particular benchmark scenarios you were running to do stress testing?

Current code is here: redux-toolkit/src/entities @ feature/entities.

Huh. One other note. Looks like the branch fails CI checks against TS 3.3 for some reason.

[brandonroberts]

Closing as resolved, but hopefully there's more collaboration in the future!

[markerikson]

Yes, absolutely! I got to watch the "State of NgRx" stream you did, and was both impressed by the amount of stuff y'all are doing and intrigued by how much of it is similar to what we're trying to do with RTK atm. We _really_ ought to have some further discussions in the near future.

[alex-okrushko]

@brandonroberts we probably need to revisit this issue. ComponentStore would also benefit from ngrx/entity 😅

[brandonroberts]

I agree. It would be super useful to have it available across both areas. Of course, I don't want to break existing users, or make it overly difficult to use it how they've been using it currently though. So if we can find a clean separation there, that would be great.

[alex-okrushko]

I'll reopen this one for now.