Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DATA] Identifiers #403

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Nov 26, 2018

@runspired runspired added the T-ember-data RFCs that impact the ember-data library label Nov 26, 2018
@jayjayjpg
Copy link
Member

@runspired Looking at the WIP tag, is it alright for us - @emberjs/the-ember-times-editors - to feature this already in next week's newsletter?

@runspired
Copy link
Contributor Author

@jessica-jordan still too many rough edges, but I think we'll be in good shape by late Wednesday/Thursday for featuring it next week.

text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
@runspired runspired changed the title [WIP DATA] RecordIdentifiers [DATA] Identifiers Feb 27, 2019
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
@courajs
Copy link

courajs commented Mar 24, 2019

I know I'm pretty late to the party here, but are we sure we don't want to take the opportunity to generalize away from string-only ids? Sometimes it is useful to use compound identifiers. I'm actually using them in an (experimental, without ED) app right now - {site_id: '<a uuid>', index: <an integer>}.

Although these can be serialized to/from strings, it places restrictions on the individual sub-keys (no colons, or must remain a fixed length, etc).

Maybe the cost is too great (need to define an equality protocol, makes maps/caches/lookups harder & more expensive, etc), but I'd love to hear any thoughts on the subject.

text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
text/0000-ember-data-identifiers.md Outdated Show resolved Hide resolved
@runspired
Copy link
Contributor Author

@courajs

I know I'm pretty late to the party here, but are we sure we don't want to take the opportunity to generalize away from string-only ids? Sometimes it is useful to use compound identifiers. I'm actually using them in an (experimental, without ED) app right now - {site_id: '', index: }.

Compound identifiers still need to be serializable and (more importantly) would not provide a path for resolving a large number of the edge cases this RFC tackles (mainly around polymorphism, alternate indexes, new record creation, and race conditions during save).

Moreover (and not to nitpick, but I feel this is important) in the example compound key you gave ({site_id: '<a uuid>', index: <an integer>}), only one of the two properties is actually required. If site_id is truly a uuid, then it is unique across types and not merely within the space that index represents. Obviously, the more common case here is that site_id is not a uuid.

Although these can be serialized to/from strings, it places restrictions on the individual sub-keys (no colons, or must remain a fixed length, etc).

I feel just the opposite is true here.

First: we don't attach an arbitrary length to the lid requirement, nor does it propose any meaning around delimiters within the string (such as colons). All we ask is that given some data, you tell us how to identify. The specifics around what guarantees you need to provide on key-uniqueness are up to you.

Second: While you could attempt to parse your compound properties out of the lid, you very much should not, and we intend to obfuscate the lid we generate a bit to help folks avoid trying to parse type and id information out of our default strategy (vs just using type and id which are also available for instance).

Maybe the cost is too great (need to define an equality protocol, makes maps/caches/lookups harder & more expensive, etc), but I'd love to hear any thoughts on the subject.

It would most definitely make a lot of costs greater, lookups harder, equality checks difficult. More importantly it would make serializing the state of things unnecessarily difficult.

Consider the case where the compound key used to generate the lid is not 1 or 2 keys but N keys. Ultimately for us to ask you if your object is arbitrarily equal to another object that may not have all N keys would be impossible. However, asking you if an object with a serialized form of identity is equal to an object with all N keys but where the serialized form is not present is very achievable.

@sandstrom
Copy link
Contributor

Very excited about this! As someone who has followed the lid discussion in JSON-API for a while, I think this would be a great improvement to Ember Data! 🏅

@runspired runspired force-pushed the ember-data-identifiers branch from fd71f0a to c71bb98 Compare April 4, 2019 19:49
@runspired
Copy link
Contributor Author

At our meeting yesterday we voted to merge this RFC. Having squashed the commits and renamed the file with the RFC # and updated the appropriate links, I am now merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-ember-data RFCs that impact the ember-data library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants