add back idAttribute method

[lukekarrys]

This adds back the getIdAttribute() method that existed in v1.0.0.

This is helpful for me when using with a library like redux-crud where it also needs the idAttribute passed in as an option. With this change I can create my schema as usual, and then read the idAttribute and pass it directly to the redux-crud reducer for each entity.

[gaearon]

I'm not opposed to adding it back but then we need to make it a documented public API, or we'll likely break it again. However currently schemas don't have any "introspection" public API at all. Now that we're at it, do you think it's worth adding any other methods to the schemas for third-party libraries?

[lukekarrys]

I'm also using the undocumented getKey method, which is enough for me (at least for EntitySchema as I'm not currently using IterableSchema).

I can add documentation for getKey and getIdAttribute, and I might as well and a test for getKey like I did for getAttribute to avoid regressions or removal.

[lukekarrys]

I've added docs for those two methods and a few more lines of tests. I'm open to adding more methods if anyone has further needs, but these feel like enough to me for EntitySchema.

[gaearon]

Do you have any problems with the case where idAttribute is a function?

[gaearon]

Out in 2.0.0.

[alexandertrefz]

@paularmstrong This method seems to have disappeared again in v3.x - is there a reason for that and can we get it back?

[paularmstrong]

@atrefz I don't understand the use-case in continuing to support this as a public method. Normalizr 3.0 includes breaking changes that would make this function not very usable outside of the normalization process. The equivalent of idAttribute, getId takes three arguments, which you're not likely to have the second and third.

[alexandertrefz]

getId is not equivalent to getIdAttribute - since the former gets the id value while the latter gets the name for the key that holds that value.

This is useful for libraries that wrap around normalizr, and want to offer abstractions.

In my specific use case: I am maintaining a generic-api package that lets you define schemas with normalizr among other things. The packages offers one specific abstraction that allows you to just query { id: <id> } of a given type - we map id to whatever the actual idAttribute of that type is, with the help of getIdAttribute (we rewrite the query that gets sent to the server).
I am sure there are other use-cases where somebody would need to know idAttribute rather than just having a way to access id, like the example that is pointed out in the original comment for this PR.

Now, I understand that _getId can be a function now, rather than just a key, and maybe getIdAttribute could return the function in that case (since that is what is passed as options.idAttribute) - I am not sure about the details & reasons here, but that would seem like a reasonable solution to me.

[paularmstrong]

@atrefz if you're writing an abstraction on top of normalizr, why don't you do just that?

import { schema } from 'normalizr';

export class Entity extends schema.Entity {
  getIdAttribute() {
    return this._getId;
  }
}

You may have to change what's getting returned from time to time, but I don't think I feel comfortable supporting getIdAttribute due to the nature of the arguments that it can require.

[alexandertrefz]

@paularmstrong

First of all I need to clarify, while this project is abstracting some things that have to do with normalizr it still expecting normalizr schemas as its input - the generic-api is not itself defining the schemas, so we can't extend the class like you suggest(without a lot of wrapping and unwrapping at least).

More importantly however: _getId is still not idAttribute - if the user passes in his own function it is, but if he just a string _getId is a function that closures over the idAttribute and prevents access to it (this includes the notion of adding a method to the prototype/an instance, or wrapping an instance of schema.Entity)

We don't need the id value, we already got that under the name id but we want to rewrite our data to have the correct name for the id value.

We want to transform like this:

let data = { id: 'foo' } 
let correctedData = { [schema.getIdAttribute()]: data.id }

There really is no reason to prevent users from accessing this key and it is clearly useful to multiple open source projects. The solution I was about to propose is incidentally exactly how this was implemented before:

https://github.com/paularmstrong/normalizr/blob/460b18280641e959f2936e551d01a43af4eb5144/src/EntitySchema.js

Simply saving options.idAttribute as _idAttribute and giving a public accessor that returns it as is, is exactly how it has worked before.

This seems like no effort whatsoever and I don't see any support effort either - you just pass back what you got as an option. That those 2 things are in direct correlation is obvious to anybody using this method IMHO.
Additionally: getId is a public method that supports 3 arguments - the exact same that options.idAttribute (possibly) takes, so why would it not be ok for another public function to return that function?

If that still makes you uncomfortable you could make it a private method _getIdAttribute. Both those things are fine, but with the current implementation there is no way of accessing options.idAttribute since you are closuring over it exclusively, removing the possibility of abstracting the fact that id can have different names for any library that uses normalizr, for what seems to be no good reason.

[paularmstrong]

Ah, I understand now. Thanks for clarifying (and sorry I'm a little thick-headed 😉).

I'd accept a PR that implements get idAttribute() { return this._idAttribute; } or whatever. (I'd rather make it strict getter than a function with a prefix)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.