ember-graphql / ember-apollo-client

🚀 An ember-cli addon for Apollo Client and GraphQL
MIT License
279 stars 72 forks source link

Added tracking to POJO returned when assigning new data #398

Closed kobsy closed 3 years ago

kobsy commented 3 years ago

I was running into an issue (possibly related to #384) where native getters on my controllers would not recompute their values when the Apollo cache was updated. This became especially apparent when using an API that took advantage of relay-style pagination: a computed property that pulled out all the edges' nodes on a field would not update when either the node was changed or a new edge was added (e.g., as part of a create mutation). What I found particularly odd was that I could make the update work if I added a @computed decorator to the getter. So, for example:

// This did not update when the underlying cache updated
get reviews() { return this.model.movie.reviews.edges.map(edge => edge.node); }

// ... but this did!
@computed('model.movie.reviews.edges.@each.node')
get reviews() { return this.model.movie.reviews.edges.map(edge => edge.node); }

Because I could get it to work using the decorator, it seemed like there was something that wasn't getting set up for autotracking using @tracked when the data was initially assigned, resulting in setProperties() doing nothing until something registered that it was watching the value via the classic observation system.

Thankfully, the solution seems, at least on the surface, to be pretty simple: we can utilize tracked built-ins to have our initial POJO actually be a TrackedObject, which hooks it into the modern @tracked autotracking system. This way, even a getter on a Controller will register that it consumes the data, and a refetch will trigger an update. 🎉

(When attempting to write the test, my first attempt utilized a Component to pull the reviews out of a movie and display the star ratings; however, assigning the movie as an argument to a Component happened to register it with the autotracking system (I'm guessing), since Component arguments are automatically auto-tracked. The tests passed before any changes were made to the rest of the code! 😅 This would mean that for users who only pass fetched data directly down to a Component before transforming/consuming, they might not ever encounter the issue this PR is attempting to address, since it would appear to "just work" in that case.)

Thanks for your feedback and consideration! Fantastic (and indispensable) addon! 😁

kobsy commented 3 years ago

Looks like the failing tests are due to a requirement to support IE11 (which tracked-built-ins doesn't)? Not sure how flexible you are willing to be on that point; if the addon needs to continue to support IE11, then this route simply won't work for fixing the underlying issue. 😞

josemarluedke commented 3 years ago

Hi @kobsy! Thanks for looking into this! I don't think we should drop IE11 support here.

I have been working on a new implementation for Ember (and Glimmer) with Apollo. Glimmer Apollo, which takes a different approach using the Use Resource pattern. That project should have your case covered because it is all auto-tracking based. The project is still very early, just FYI.

Now, looking specifically at your issue, what I have been doing in projects is using more components instead of the router models hook. With components, I usually have a tracked property that I set the values on from the response. However, it doesn't really solve your issue because you want to rely on apollo's cache and that's exactly why I started looking for alternatives to the current setup of ember-apollo-client.

Anyway, let me know if you wish to look into solving this issue in this project in another way.

kobsy commented 3 years ago

Thanks for the feedback! Glimmer Apollo looks promising! 😁 I completely understand the lack of IE11 being a show-stopper for this solution. I thought I'd take one more crack at it, though, so I opened #399, which would supersede this PR. I'd appreciate whatever feedback you might have if you get a chance to look at that one, as well. 🙂 Thanks!