canjs / can-connect

Model layer utilities for every JavaScript framework! Assemble real-time, high performance, restful data connections.
https://canjs.com/doc/can-connect.html
MIT License
29 stars 16 forks source link

`gotInstanceData` method wiring is missing from the data/callbacks behavior #301

Open pmgmendes opened 7 years ago

pmgmendes commented 7 years ago

How often can you reproduce it?

Description: Although the documentation can-connect references a callback named gotData this method is not being wired to getData in the data/callbacks pairs so it can be overwritten by other behaviors. The pairs object in can-connect/data/callbacks/callbacks.js is expected to also have

var pairs = {
  ...
  getData: "gotInstanceData",
  ...
};
nlundquist commented 7 years ago

The reference you see to gotData on the main can-connect documentation index page is outdated. This will be corrected shortly with a major documentation update that is underway.

The docs for can-connect/data/callbacks here correctly show createdData, destroyedData, gotListData & updatedData. These are the only callbacks that expected to be part of this behavior at this time.

I'll report back if we have a timeline for adding a gotData callback to the data/callbacks behavior. In the meantime the best workaround would be to create your own behavior that wraps getData and does whatever transformation to the request or response you require.

justinbmeyer commented 7 years ago

@pmgmendes or ... submit a pull request with it fixed and we can have a release out immediately :-)

pmgmendes commented 7 years ago

@justinbmeyer So it should be there? Yeah, I'll do it.

justinbmeyer commented 7 years ago

yeah, should be there. Btw, what are you using it for?

pmgmendes commented 7 years ago

I'm accessing a resource modeled as /api/resource/{id} which returns a simple key value map

{ "keyA": "valueA", "keyB": "valueB", ... }

This is a resource that returns only static data from the server (but might change after new re-deploys) and I want to cache the response in localStorage but the returned map doesn't have a unique identifier. So I've created the instance type as a define map with the following props

{ 
id: "string",
data: {}
}

And in gotInstanceData I intend to do something like

gotInstanceData: (response, params) => {
  return {
    id: params.id
    data: response
  }
}

This way I can make use of the behaviors that use instanceStore too. It might appear that I could be better off using cacheRequests behavior but I still want to perform those requests to the server so I'm using fallThroughCache strategy.

pmgmendes commented 7 years ago

@justinbmeyer I've added the gotData getData pair to data/callbacks and also included the getDatain the method list for validation. Some tests in real-time behavior fail

Error: can-connect: Extending behavior "data/callbacks" found base behavior "real-time" was missing required properties: ["getData"]

Should real-time implement getData? I've done it but I'm struggling a bit to have the tests properly evolved to support getData. Help needed.

justinbmeyer commented 7 years ago

For this use case, would parse-data do what you need?

Sent from my iPhone

On Jun 6, 2017, at 4:03 PM, Pedro Mendes notifications@github.com wrote:

I'm accessing a resource modeled as /api/resource/{id} which returns a simple key value map

{ "keyA": "valueA", "keyB": "valueB", ... } This is a resource that returns only static data from the server (but might change after new re-deploys) and I want to cache the response in localStorage but the returned map doesn't have a unique identifier. So I've created the instance type as define map with props

{ id: "string", data: {} } And in gotInstanceData I intend to do something like

gotInstanceData: (response, params) => { return { id: params.id data: response } } This way I can make use of the behaviors that use instanceStore too. It might appear that I could be better off using cacheRequests behavior but I still want to perform those requests to the server so I'm using fallThroughCache strategy.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

pmgmendes commented 7 years ago

I need the request params and parseData just provides the response data. In the meantime I was able to overwrite getData as suggested by @bmomberger-bitovi and @nlundquist in Gitter to attain the same purpose.

const customBehavior = function (baseConnection) {
  return {
    getData: function (params) {
      return baseConnection.getData(params).then(function (instanceData) {
        return {
          id: params.id,
          data: instanceData
        };
      });
    }
  };
};

Todo.connection = connect([ ..., dataUrl, customBehavior, dataParse, ...], {...});
nlundquist commented 7 years ago

Good to hear @pmgmendes! If you want to open a PR for your branch I can help you resolve the problems you're having with the tests. I suspect the issue you're seeing is due to the test for the real-time behavior using a "stub" behavior that only implements getListData instead of the full DataInterface. Adding an empty function called getData to that stub behavior should fix the test.