search

beauby / jsonapi-datastore

JavaScript client-side JSON API data handling made easy.
MIT License
195 stars 40 forks source link

implemented _links property to hold link and relationship links while… #17

Open amwmedia opened 8 years ago

amwmedia commented 8 years ago

Here's a possible implementation for _links. There may be a better way to do this, but this was the most obvious solution I could think of that doesn't break backwards compatibility by turning the relationships array into an array of objects (or something like that).

beauby commented 8 years ago

Hi @amwmedia, thanks for your work (again ;)). A few remarks I'd like to hear your thoughts about:

  1. Could you add some tests to ensure that resource-level and relationship-level links are parsed as we expect, and also a test for what happens when no link is provided (I believe with your current patch the _links attribute would not be defined, but it might be better to set it to {} by default).
  2. What happens if both a resource and one of its relationships have a self link? I believe currently it would lead to the resource's self link being overwritten. Maybe a solution would be to have a separate _relationship_links property.
  3. There should be no need to name the property _links rather than simply links as the spec formally disallows attributes named links. edit: however, seeing that we already have an _attributes property, it makes sense to keep it as _links.
amwmedia commented 8 years ago

Hey @beauby, I didn't want to finalize this too much (with tests and such) before I got your take on the actual implementation. Currently _links is created, and relationships links fall into a _links.relationshipName object. I think this works so long as the spec does not allow for relationships within relationships (which I don't believe is the case). The only possible conflict with this approach would be if you have a relationship named "self" or something unlikely like that.

We could prefix a relationship link property with _ as well. so you'd end up with _links._relationshipName

Thoughts?

kulor commented 8 years ago

hey, @amwmedia thanks for this, I'm keen to use the links functionality so am willing to help out with this if you need it (write tests etc).

I've just checked it out locally and it seemingly works with my heavily linked dataset.

on the discussion about _links vs. links, I vote to stay with _links as it's the internals of the model than the first class attributes.

cc/ @beauby

beauby commented 8 years ago

Hi there! Sorry I took so long (again) to get back. So, after giving it a thought, I definitely agree that _links is more consistent than links. The only thing I'm not comfortable with in this PR is merging the resource and relationship links. I'd much rather have a _relationship_links object storing links for each relationship. Other than that, I'm totally :+1: on this PR and would love to get it merged with some tests!

amwmedia commented 8 years ago

the possibility of a relationship link collision was my only reservation on this as well. I can try to get this change in soon. We are closing out a milestone today at work, so possibly early next week.

beauby commented 8 years ago

Awesome @amwmedia!

beauby commented 8 years ago

Hi @amwmedia, any news on this front?

amwmedia commented 8 years ago

Sorry, things did not slow down as I had hoped after that week. In fact, they got more busy :-). This is still on my radar, but if anyone else wants to make this last small change, feel free. I've just been swamped lately.

vjustov commented 7 years ago

Hey @amwmedia, Did you get a chance to check this out?