Akita: Support nested idKey in Entity Store

[Mautriz]

I'm submitting a...

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

Current behavior

Allows only for idKey to be on the top level of the arriving object

Expected behavior

To be able to use a nestedKey in the configuration ex: { idKey: 'snippet.id.videoId' }

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

Simplifies working with those kinds of objects, it happened to me while trying out the YouTube Data API, which had the videoId key nested iniside 'snippet.id.videoId' as shown above, so I had to map the items every time to put the key on the top level and use it with akita

Others:

[NetanelBasal]

You're welcome to submit a PR.

[j2L4e]

@NetanelBasal Do you see any obvious problem with the user passing in an id selector function?
e.g.

idKey: entity => entity.id.videoId

This would also allow to use entities with composite keys (which is what I was actually looking for here) or any other smart (or not so smart) use-case someone might come up with.

idKey: entity => `${entity.versionId}:${entity.id}`

[NetanelBasal]

I can't see any problem, and that's why I'm accepting a PR :)

[thayesx]

@j2L4e are you working on a PR for what you described above? i'd love to see this feature too

[NetanelBasal]

I don't think so. Feel free to take it.

[j2L4e]

@thayesx I started working on one, see https://github.com/j2L4e/akita/tree/id-selector
Added getId and setId functions to StoreConfig, because it has to work both ways (i.e. id to object and object to id). It doesn't get the job done yet, though.
I won't be able to get back to working on this any time soon, so feel free to give it a shot.

If I remember correctly, problems I had were mainly with uiStore creation and upsert

[thayesx]

thanks @j2L4e! I like your getId/setId implementation, makes sense. i'll dig into this

[gioragutt]

@NetanelBasal is this solved already? we could really use this in our code 😁

[NetanelBasal]

No, I'm still waiting for a PR :)

[gioragutt]

@NetanelBasal unfortunate 😔 we'll try to find a workaround, and if not, we would probably not use Akita.

I could work on a PR.

Any guidance on things I should pay attention to?

Something that could possibly slip out if I'm not too familiar with the codebase?

[NetanelBasal]

Check this comment: https://github.com/datorama/akita/issues/387#issuecomment-647370323

[gioragutt]

@NetanelBasal I'm starting the PR, so I'll try to upload something soon.

I'm seeing lots of tslint warnings (such as interface-over-type-literal). Is it that my vscode may be configured differently or should these things actually be fixed?

[NetanelBasal]

Ignore it.

[gioragutt]

@NetanelBasal regarding idKey.

It's clear from the code that the idea of getId and setId is needed, because it's no longer property access.

I've looked in the docs, and other than StoreConfigOptions, the idKey is not part of any public api (at least nothing without @internal).

I was thinking whether or not idKey should stay in StoreConfigOptions, and only be converted to getId and setId if they aren't provided.

This would also require some input handling in StoreConfig to ensure that either both getId and setId, or idKey, are provided.

Also, I saw the Array Utils api, and I wonder if we need to accommodate these utilities when there's not a simple key involved?

I'm sure there might be more APIs that work with ids, I wanna make sure nothing is left behind.
This is obviously a complex change, considering the codebase expects the id field via property access.

Your thoughts?

[NetanelBasal]

Sounds good.

[j2L4e]

I'm sure there might be more APIs that work with ids, I wanna make sure nothing is left behind.
This is obviously a complex change, considering the codebase expects the id field via property access.

This might be a proper way:

1. remove idKey completely (or throw an error in its getter and setter to make sure it's unused during tests)
2. refactor everything to a getId/setId implementation so that it passes the current test suite
3. reintroduce idKey as a compat-layer (or just drop it altogether as a breaking change)

---

To make getId/setId reusable more easily, this API might actually preferable over separate getId/setId props

idKey: {
  get(entity) { /* */ },
  set(entity, key) { /* */ },
}

Would also make the compat-layer rather easy:

if(isString(idKey)) {
  idKey = {
    get: entity => entity[idKey],
    set: (entity, key) => ({ ...entity, [idKey]: key }),
  }
}

Arguably, what you get and set is the id rather than the idKey, so the naming might be a little off here.

---

Another thought: Should setId mutate entity (i.e. return void) or return a clone?

[gioragutt]

@j2L4e I don't think that dropping the idKey is the best idea, since for most cases (I guess), this is sufficient.

We can do something like:

export type IDKeyConfig = string | {
  getId(entity: any): ID;
  setId(entity: any, id: ID): void
};

export interface StoreConfigOptions {
  name: string;
  resettable?: AkitaConfig['resettable'];
  cache?: { ttl: number };
  deepFreezeFn?: (o: any) => any;
  idKey?: IDKeyConfig;
  producerFn?: AkitaConfig['producerFn'];
}

Another thought: Should setId mutate entity (i.e. return void) or return a clone?

Considering everywhere that entity[this.idKey] has been used now has been mutating, I think it should stay that way

[NetanelBasal]

I'm thinking about something like:

export interface StoreConfigOptions {
  name: string;
  resettable?: AkitaConfig['resettable'];
  cache?: { ttl: number };
  deepFreezeFn?: (o: any) => any;
  idKey?: string | ((entity) => string));
  producerFn?: AkitaConfig['producerFn'];
}

Then in the store you create resolveId function:

resolveId = isString ? entity => entity[idKey] : idKey

Now you can use it everywhere instead of entity[this.idKey] you use entity[resolveId(entity)]

[gioragutt]

@NetanelBasal

Now you can use it everywhere instead of entity[this.idKey] you use entity[resolveId(entity)]

The problem is that entity[resolveId(entity)] isn't possible considering the change we're trying to make

[NetanelBasal]

What do you mean isn't possible?

[j2L4e]

resolveId = isString ? entity => entity[idKey] : idKey

Now you can use it everywhere instead of entity[this.idKey] you use entity[resolveId(entity)]

This would only work in the simplest of all cases: A non-nested single property. I don't see any benefit over the current string idKey implementation. With a setId/getId implementation the id could be pretty arbitrary. Multi-prop, nested and what not.

I don't think that dropping the idKey is the best idea, since for most cases (I guess), this is sufficient.

@gioragutt My thought was to only drop it temporarily for refactoring, because you "wanna make sure nothing is left behind"

[NetanelBasal]

Oh, I forgot the use case :)

@j2L4e https://github.com/datorama/akita/issues/387#issuecomment-641979411

[NetanelBasal]

But why not use a pre-hook?

preEntityAdd(entity) {
  return {
    ...entity,
    id: entity.some.nested.key
  }
}

[gioragutt]

But why not use a pre-hook?

preEntityAdd(entity) {
  return {
    ...entity,
    id: entity.some.nested.key
  }
}

Honestly? since I didn't use Akita before, only read the docs several times, I didn't get too much into hooks.
I'll take a look, and see if this fits our use case.

If it does, I'll be happy to update the docs to give some clues for people like us.
Question is whether the places that update the id (the whole need for setId) aren't the edge case for this.

[NetanelBasal]

I'm pretty sure you can get around with the above solution. You are creating the idKey on the root (where it needs to be) and it should work. If you want to name it differently you can:

preEntityAdd(entity) {
  return {
    ...entity,
    tempId: entity.some.nested.key
  }
}

Then in the store:

StoreConfig({ idKey: 'tempId' })

[gioragutt]

From the replace method, which is the only place that uses this:

replace(ids: IDS, newState: Partial<EntityType>) {
    // ...
    for (const id of toArray) {
      newState[this.idKey] = id;
      replaced[id] = newState;
    }
    // ...
  }

Isn't the reuse of the newState variable a bug?

I mean, yes, the idKey would in-fact work here. But this still seems like a pretty nice bug 😅

[NetanelBasal]

Question is whether the places that update the id (the whole need for setId) aren't the edge case for this.

For example?

[NetanelBasal]

Isn't the reuse of the newState variable a bug?

Why a bug? It's a brand new object you pass.

[NetanelBasal]

Try the hook version first, and let me know how it goes.

[gioragutt]

Isn't the reuse of the newState variable a bug?

Why a bug? It's a brand new object you pass.

Since you reuse the object, and the same object gets passed to all the replaced[id], they all end up having the last id from the array, no?

Edit: what might happen here is that the entities sit in the mapping under the correct id, but they will have the same id in the value (those entities that were replaced).