mattkrick / cashay

:moneybag: Relay for the rest of us :moneybag:
MIT License
453 stars 28 forks source link

Wires crossed with CachedQuery data objects #144

Closed dustinfarris closed 7 years ago

dustinfarris commented 7 years ago

I am running into a strange scenario where the data in a cached query looks like (but is oddly not an instance of) Ember.Object. It has getters and setters and meta properties just like an Ember.Object. I am noticing this specifically in the mutation handler currentResponse param:

CachedQuery objects are internal to cashay so I am astonished that I am seeing something that looks like Ember.Object.

I am investigating where this overlap is occurring. I assume the problem is on my end, but I noticed this line in the action project which made me think there might be a larger issue here.

mattkrick commented 7 years ago

That's really weird! I can't comment since cashay doesn't do any ember stuff but I'd you figure it out lemme know

On Dec 4, 2016 2:05 PM, "Dustin Farris" notifications@github.com wrote:

I am running into a strange scenario where the data in a cached query looks like (but is oddly not an instance of) Ember.Object. It has getters and setters and meta properties just like an Ember.Object. I am noticing this specifically in the mutation handler currentResponse param:

https://camo.githubusercontent.com/032a2b00912316e2d39f31495e466e25e0a6ce02/68747470733a2f2f7777772e64726f70626f782e636f6d2f732f7868687a376a646471706f326368632f53637265656e73686f74253230323031362d31322d303425323031372e30342e31362e706e673f646c3d31

CachedQuery objects are internal to cashay so I am astonished that I am seeing something that looks like Ember.Object.

I am investigating where this overlap is occurring. I assume the problem is on my end, but I noticed this line in the action project https://github.com/ParabolInc/action/blob/47e3310bd14b9896ab163e19dc0731d23cd61aaf/src/universal/modules/summary/containers/MeetingSummary/MeetingSummaryContainer.js#L46 which made me think there might be a larger issue here.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mattkrick/cashay/issues/144, or mute the thread https://github.com/notifications/unsubscribe-auth/AFQjv3Y5X8BT0wAeagoIJ3FpVCGP-J2zks5rEzlBgaJpZM4LDtvX .

dustinfarris commented 7 years ago

I think I figured this out.

In my stateToComputed (our version of stateToProps in react-redux), I do this:

stateToComputed = (state, { projectId }) => {
  const { data: { project } } = cashay.query(projectQuery, { variables: { projectId } });
  return { project };
}

With project now available as a computed property, I can use it in the template:

{{project-detail project=project}}

As soon as I pass it as an attr to another component like this, Ember "binds" it which involves transforming the project object into one that signals Ember whenever it changes. It does this by adding meta data, adding getters and setters—basically hi-jacking the POJO. This affects redux state! It is also why Object.assign doesn't work.

It seems the only way around this is to deep copy the data before returning it in stateToComputed, e.g.:

  const { data: { project } } = cashay.query(projectQuery, { variables: { projectId } });
  return {
    project: jQuery.extend(true, {}, project)
  };

I am going to open an issue in ember-redux to address this.

Update: see https://github.com/toranb/ember-redux/pull/58

mattkrick commented 7 years ago

oh wow, i didn't know ember worked like that! yeah, the redux state should be immutable, so if ember is going to try & mutate it, you'll wanna put something between the redux state & the view layer, otherwise you'll end up having to deep copy an object on every re-render which will get expensive.