select() and selectAll() don't have the same behavouir

[RobinVdBroeck]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ X ] Other... Please describe: Disscusion

Current behavior

select('selector') starts at the root store.

Example select('foo') on a store with pseudo-value:
{ foo: 0, entities: [{ key: 'foo', value: 1 }] } will return 0.

However, selectAll({limitTo: 1, filterBy: (entity) => entity.key=== 'foo'}).pipe(pluck('value') will return 1.

If you want the expected behavouir, you need to use selectEntity.

Expected behavior

Either select should start at 'entity' level, or selectAll() should be renamed to selectAllEntities() to make it clear we are talking about entities.
This makes it clear that code like this.selectAll({limitTo: 1, filterBy: (entity) => entity.key=== 'foo'}).pipe(pluck('value') cannot be simplified to this.select('foo').

What is the motivation / use case for changing the behavior?

We had a real life bug because of this. The api is not clear.

[NetanelBasal]

selectAll exists only in the entity store, and returns an array. The usage of pluck doesn't make sense in this case.

[RobinVdBroeck]

It doesn't make sense that selectAll({limit: 1}) and select() don't behave the same way.

[NetanelBasal]

selectAll returns the entities collection. select() returns a specific slice from the store.

[RobinVdBroeck]

selectAll returns the entities collection. select() returns a specific slice from the store.

That is not clear for a consumer of the api. Akita should adhere to the principle of least suprise.

Another "solution" would be to have select more strictly typed. Akita should know which store interface it is querying, so it should be able to typecheck that it exists. Ie, in pseudocode:

interface State {
  bar: string;
  delta: number;
}
const store: State = {
  bar: 'x',
  delta: 2
};
store.select('foo'); <-- this can be a compile error, since we know 'foo' is not a key of store (which is 'bar' | 'delta')
declare key: string;
store.select(key); <-- this is also a compile error since string is not a 'bar' | 'delta'

[NetanelBasal]

[RobinVdBroeck]

It does not give a compile error for our code. We'll investigate and come back.

[RobinVdBroeck]

@david-bulte found the issue. EntityState is defined as follows in the code:

export interface EntityState<E = any, IDType = any> {
  entities?: HashMap<E>;
  ids?: IDType[];
  loading?: boolean;
  error?: any;
  [key: string]: any;
}

However, if we remove that [key:string]: any, select works as it should. See this stackblitz for a working example. I'm willing to start a PR where we remove the {[key:string]}: any, if you're happy with that change.

[NetanelBasal]

Sure

[Yash636261]

I think using .select + .data should not be a valid use case anyway. .select seems to create a selection with parentNode being (the HTML root node) and .selectAll a selection with (after .select('body')). Appending the elements to html does not seem to be the expected behaviour, but it could be intentional. I'd ask the author, it could be a bug as well.