entitiesPropsFactory: return CRUD functions similar to propsFactory

[DmitryEfimenko]

propsFactory returns convenience functions to update the value in the store:

export const {
  withVersion,
  updateVersion,
  selectVersion,
  resetVersion,
  getVersion,
  setVersion,
} = propsFactory('version', {
  initialValue: 1,
});

I wonder if something similar would make sense for entitiesPropsFactory:

const {
  itemEntitiesRef, // don't need it really if below functions are available
  withItemEntities,
  updateItemEntities,
  selectItemEntities,
  selectAllItemEntities,
  resetItemEntities,
  getItemEntities,
  setItemEntities,
} = entitiesPropsFactory('item');

using itemEntitiesRef with all crud functions and selectors works, but having the functions above available would make code a bit easier to read:

items$ = store.pipe(selectAll({ ref: itemEntitiesRef }));
// vs
items$ = store.pipe(selectAllItemEntities());

[NetanelBasal]

It's a nice idea, but there are two advantages:

• Because you're exposing an object with all methods, tree shaking will be broken.
• You'll have to maintain it

[DmitryEfimenko]

I thought tree shaking might be an issue. I didn't understand the second point though. What's "it"?

[NetanelBasal]

Each new mutation/selector

[NetanelBasal]

You can always create a third-party lib with your suggestion, and I'll link it from the docs.

[DmitryEfimenko]

per your comment about the tree shaking, I don't think it's worth it. Using itemEntitiesRef is not all that bad.

[DmitryEfimenko]

getting back to this idea as I thought of a potentially different approach. What if we allowed the following:

import {
  entitiesPropsFactory,
  addEntities,
  selectAll,
  setEntities,
  updateEntities,
  selectEntity
} from '@ngneat/elf-entities';

const { todoEntitiesRef, withTodoEntities, todoCrudFactory } = entitiesPropsFactory('todo');

const { addTodoEntities, selectAllTodo, setTodoEntities, updateTodoEntities, selectTodoEntity } = todoCrudFactory({
  addEntities,
  selectAll,
  setEntities,
  updateEntities,
  selectEntity
});

The todoCrudFactory function would have the reference to todoEntitiesRef internally and would only generate crud functions for the functions passed in. This way we keep the tree-shaking benefits. Something like:

const ref; // the reference to the todoEntitiesRef in the closure
const name; // the name passed into entitiesPropsFactory... in this example "todo"
function todoCrudFactory(args) {
  const result = {};
  if (args.addEntities) {
    result[`add${name}Entities`] = (entities, options) => {
      return args.addEntities(entities, Object.assign(options, { ref }));
    }
  }
}

The above code is super rough draft.

[NetanelBasal]

Seems reasonable to me.

[DmitryEfimenko]

So I'm working on this feature and I'm thinking about how the crudFactory function could be made smaller.
Having the if statements for each possible function (like in the draft code from my previous comment) is very verbose and repetitive. There are currently 27 functions that could be passed in. This would not be good for bundle size.

I think it'd be better if we did something like:

const featuredUpdatersMap = {
  selectAll: `selectAll${feature}`,
  selectAllApply: `selectAll${feature}Apply`,
  updateEntities: `update${feature}Entities`
  // ... etc
}

function crudFactory(updaters) {
  const res = {};
  for (const key in updaters) {
    res[featuredUpdatersMap[key]] = // ... something. This issue is discussed further
  }
  return res;
}

This is short and sweet (never mind the absence of types). The biggest part would be the featuredUpdatersMap variable containing all 27 function names. I don't see a way around that.

But now, the second issue... we need to override the function so that it's called with the correct ref. The problem with this is that the options object which contains the ref is sometimes passed in as the first (for example, in selectAll), second (for example, in addEntities), or third argument (for example, in updateEntities). Because the order of that argument is not guaranteed, I can't create a reliable generalized override.

The way I propose to solve this is by binding the { ref } object to the this context of the function:

function crudFactory(updaters) {
  const res = {};
  for (const key in updaters) {
    res[featuredUpdatersMap[key]] = updaters[key].bind({ ref });
  }
  return res;
}

We would have to update all functions to account for this. For example, here's the change in the selectAll function:

// previous code:
const { ref: { entitiesKey, idsKey } = defaultEntitiesRef } = options;

// new code
const ref = options.ref ?? this.ref ?? defaultEntitiesRef;
const { entitiesKey, idsKey } = ref;

What do you think about this?

[NetanelBasal]

To be honest, I don't think it's worth it for now.

[DmitryEfimenko]

could you elaborate?

[DmitryEfimenko]

can you please in the future be more specific to avoid additional questions?
What aspect of the proposed solution concerns you?

[NetanelBasal]

Changing the entire code and how ref works for this feature.

[DmitryEfimenko]

I see. What if we included the information about the index of the argument that contains ref in the featuredUpdatersMap:

const featuredUpdatersMap = {
  selectAll: [`selectAll${feature}`, 0],
  selectAllApply: [`selectAll${feature}Apply`, 0],
  updateEntities: [`update${feature}Entities`, 2]
  // ... etc
}

function crudFactory(updaters) {
  const res = {};
  for (const key in updaters) {
    const [fnName, optionsArgIndex] = featuredUpdatersMap[key];
    res[fnName] = (...args: any[]) => {
      const updatedArgs = args.map((arg, ix) => {
        return ix === optionsArgIndex
          ? Object.assign({}, arg, { ref })
          : arg;
      });
      return originalFn(...updatedArgs);
    };
  }
  return res;
}

I do not love it since there's now the need to maintain things in featuredUpdatersMap, but I don't think it's the worst. It would work without making changes to the existing functions.

What do you think?

[NetanelBasal]

Can you create a POC, please?