Skip to content

ember data loading methods - #27

Closed
jcollins1991 wants to merge 11 commits into
emberjs:masterfrom
jcollins1991:master
Closed

jcollins1991 wants to merge 11 commits into
emberjs:masterfrom
jcollins1991:master

Conversation

@jcollins1991

Copy link
Copy Markdown

here you go @igorT , everything is in the RFC markdown file

(sorry the history is such a mess.. have just been editing on github :S)

Rendered

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably mean a type instead of list of ids

@fivetanley

Copy link
Copy Markdown
Member

We discussed this today during the Ember Data meeting. First off, thanks for this RFC, the tables give a very good understanding of what the current methods currently do.

Here are the use cases we derived out of the RFC that we want to support going forward:

  1. Load data only from the local store, or go to the server.
  2. Load data from the local store, and refresh the record in the background
  3. Always load from the server.
  4. Query the server for its logic and load whatever models are returned.

There was some discussion about which was the use case we should highlight in the guides. There was not consensus, but that does not prevent us from implementing the different loading styles. We still need names for all the different loading styles, as we will have several primitive methods and a few higher level methods to give users flexibility in how to ask the store for data. @tomdale and @wycats will find time to look at this RFC.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't it be something like this?

multiplicity local data fetch if not local fetch always local, with new load in background
collection store.all store.find store.find (query) ?
single record store.getById store.find store.fetch ?

@eccegordo

Copy link
Copy Markdown

I think the consistency and a clear table of methods is very useful.

Like the concept of "grab". I would be curious to know if it makes sense to chain operations in order and handle fail over scenarios gracefully. e.g.

first attempt to grab from local, if nothing, then attempt to find, then fetch, and finally query conditionally.

Weaving those together semantically could be very powerful.

Imagine an iterator or pagination sequence:

  • first grab next logical record (perhaps guess at id), if nothing,
  • then find,
  • then fetch and find again,
  • if nothing last resort fall back to a dynamic query where you throw in an incremented or decremented (previous) offset or cursor position.

Lots of possibilities if there was very consistent API, and had some arbitrary control over how you wanted to attempt to load data conditionally.

@mmun mmun added the T-ember-data RFCs that impact the ember-data library label Feb 5, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also split out the function names for finding an record vs a collection. warp-drive-data/warp-drive#2607

@wecc

wecc commented Feb 10, 2015

Copy link
Copy Markdown

With find being separated into find/findAll/findQuery (warp-drive-data/warp-drive#2607) I think we need a couple of more methods to make it all work. I really like the concept of grab/find/fetch and would like to add my 2¢.

TL;DR:

Local Fetch if not local Fetch
One grab(type, id) find(type, id, [query]) fetch(type, id, [query])
All grabAll(type) findAll(type, [query]) fetchAll(type, [query])
Filtered grabFilter(type, filter) findFilter(type, filter, [query]) fetchFilter(type, filter, [query])

Long version:

{
  /**
    Use `grab()` to retreive a record by it's type and id if present in the
    store. If there's no record matching the type and id `null` will be
    returned.

    This method is synchronous.

    @method grab
    @param {String} type
    @param {String|Integer} id
    @return {DS.Model|null} record
  */
  grab: function(type, id),

  /**
    Use `grabAll()` to retreive all records present in the store, by type.

    This method is synchronous.

    @method grabAll
    @param {String} type
    @return {DS.RecordArray} records
  */
  grabAll: function(type),

  /**
    Use `grabFilter()` to retreive all records present in the store, matching
    `filter`.

    The `filter` passed to `grabFilter()` can be either an object with keys and
    values that will be used to filter the available records or a function that
    will be called once for each present record and is expected to return true
    if the record should be included or not.

    This method is synchronous.

    @method grabAll
    @param {String} type
    @param {Object|Function} filter
    @return {DS.RecordArray} records
  */
  grabFilter: function(type, filter),



  /**
    Use `find()` to retreive a record by it's type and id if present in the
    store. If there's no record matching the type and id the store will ask the
    adapter to fetch the requested record.

    An optional `query` can be passed to `find()` and in the case of the store
    asking the adapter to fetch the requested record this query will be passed
    to the adapter making it possible to use it as additional parameters in the
    server request. The query will not have any client side effect.

    This method is asynchronous.

    @method find
    @param {String} type
    @param {String|Integer} id
    @param {Object} [query]
    @return {DS.PromiseObject} record
  */
  find: function(type, id, [query]),

  /**
    Use `findAll()` to retreive all records present in the store, by type. If
    no records of the specified type is present in the store, the adapter will
    be asked to fetch them.

    An optional `query` can be passed to `findAll()` and in the case of the
    store asking the adapter to fetch the requested records this query will be
    passed to the adapter making it possible to use it as additional parameters
    in the server request. The query will not have any client side effect.

    This method is asynchronous.

    @method findAll
    @param {String} type
    @param {Object} [query]
    @return {DS.RecordArray} records
  */
  findAll: function(type, [query]),

  /**
    Use `findFilter()` to retreive all records present in the store, matching
    `filter`. If no records of the specified type is present in the store, the
    adapter will be asked to fetch them.

    The `filter` passed to `findFilter()` can be either an object with keys and
    values that will be used to filter the available records or a function that
    will be called once for each present record and is expected to return true
    if the record should be included or not.

    An optional `query` can be passed to `findFilter()` and in the case of the
    store asking the adapter to fetch the requested records this query will be
    passed to the adapter to use the parameters in a server request. The query
    will not have any client side effect.

    If `filter` is an object and no `query` is passed, `filter` will be used as
    `query` to the adapter.

    If `filter` is a function then `query` is required.

    This method is asynchronous.

    @method findFilter
    @param {String} type
    @param {Object|Function} filter
    @param {Object} [query]
    @return {DS.RecordArray} records
  */
  findFilter: function(type, filter, [query]),



  /**
    Use `fetch()` to ask the adapter to fetch a record by it's type and id.

    An optional `query` can be passed to `fetch()` making it possible to use it
    as additional parameters in a server request. The query will not have any
    client side effect.

    This method is asynchronous.

    @method fetch
    @param {String} type
    @param {String|Integer} id
    @param {Object} [query]
    @return {DS.PromiseObject} record
  */
  fetch: function(type, id, [query]),

  /**
    Use `fetchAll()` to ask the adapter to fetch all records by type.

    An optional `query` can be passed to `fetchAll()` making it possible to use
    it as additional parameters in a server request. The query will not have any
    client side effect.

    This method is asynchronous.

    @method findAll
    @param {String} type
    @param {Object} [query]
    @return {DS.RecordArray} records
  */
  fetchAll: function(type, [query]),

  /**
    Use `fetchFilter()` to ask the adapter to fetch all records by type and
    filter.

    The `filter` passed to `fetchFilter()` can be either an object with keys and
    values that will be used to filter the returned records or a function that
    will be called once for each returned record and is expected to return true
    if the record should be included or not.

    An optional `query` can be passed to `fetchFilter()`. This query will be
    passed to the adapter to be used as parameters in the server request. The
    query will not have any client side effect.

    If `filter` is an object and no `query` is passed, `filter` will be used as
    `query` to the adapter.

    If `filter` is a function then `query` is required.

    This method is asynchronous.

    @method fetchFilter
    @param {String} type
    @param {Object|Function} filter
    @param {Object} [query]
    @return {DS.RecordArray} records
  */
  fetchFilter: function(type, filter, [query]),
}

@trabus

trabus commented Feb 11, 2015

Copy link
Copy Markdown

This is great. I love the explicitness of the actions.

@funtusov

Copy link
Copy Markdown

I'd add the optional [query] parameter to the grab functions so that all request format are the same, only the source being the difference. And that will support the case of searching the local store, should someone want to do this.

@jonathanKingston

Copy link
Copy Markdown
Contributor

The ability to filter results from a collection to a single object was something I wanted to raise as an RFC however may slot better in this.

My suggestion was going to be:

  var res = this.store.findFirst('post', {author: 'Jonathan'});
  var res = this.store.findLast('post', {author: 'Jonathan'});

  //Rather than:

  var res = this.store.find('post', {author: 'Jonathan'}, function (posts) {
     return posts.get('firstObject');
  });

This would make it consistent with many other frameworks however I would also want this for grab, find and fetch methods mentioned here. Related to: warp-drive-data/warp-drive#2584

@wycats

wycats commented Feb 23, 2015

Copy link
Copy Markdown
Member

I like this a lot but prefer peek to grab. Any objections?

@dgeb

dgeb commented Feb 23, 2015

Copy link
Copy Markdown
Member

FWIW, I use retrieve in Orbit.js to synchronously access records from a source (equivalent to grab here).

@wecc

wecc commented Feb 23, 2015

Copy link
Copy Markdown

I'd be fine with both of your suggestions, though I think I'd prefer retrieve over peek (even if I find it harder to spell, Like a Swede)

We should also consider changing the behavior of find (using the same terminology as in my proposal above) to "grab then fetch in background" instead of "grab, fetch if not available".

@mitchlloyd

Copy link
Copy Markdown

My initial impression of peek is that it's not as expressive as grab. To me peek makes more sense in context of working with a stack metaphor where one is using push, pop, shift and unshift. I don't think of the store or a collection of models as a stack because I can access models by their ID.

This mental model of peek is coming from my exposure to Ruby and Clojure.

http://ruby-doc.org//core-2.2.0/Enumerator.html#method-i-peek
https://clojuredocs.org/clojure.core/peek

grab seems to fit some anthropomorphized mental model I have of the store grabbing data that it has at arms reach.

@trabus

trabus commented Feb 23, 2015

Copy link
Copy Markdown

grab seems to fit some anthropomorphized mental model I have of the store have where the store is grabbing data that it has at arms reach.

To me grab makes much more sense. You only want a record that is already 'within reach' locally, so you grab it (give me local or give me nothing). You don't have to go find it (don't care where the record comes from, just find one), or fetch it from the server (if you want a fresh record or know you don't have it locally).

@cibernox

cibernox commented Mar 4, 2015

Copy link
Copy Markdown
Contributor

I found myself again in the need of a method to give me what I have and refresh in background (the last method in the table)

Historically I've found myself writing model hooks that look like this:

model() {
  let localResults = this.store.all('post');
  let serverQuery = this.store.findAll('post');
  return localResults.get('length') === 0 ? serverQuery : localResults;
}

What could be a good name for that method? store.findRefresh('post')? just store.refresh('post')? store.backgroundFetch ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need better support for load, with new load in background. I know that @cibernox suggested that on warp-drive-data/warp-drive#2590 and I found that to be a really common pattern.

@bmac

bmac commented Jun 6, 2015

Copy link
Copy Markdown
Member

The Ember Data team has been discussing this issue over the past week. The general consensus a preference for Record instead of byId since it describes what is being fetched.

There is also a consensus around removing the fetch apis. The reasoning for this move is a fear that having both a find and fetch api may be confusing to new developers. In its place, Ember Data will be making the findRecord and findAll methods smarter so they can take an option to force the store to reload the record if needed. Additionally, the adapter will be given hooks described in RFC #61 to allow it make smarter decisions about reloading records.

Here is the currently propose store api for loading records in Ember Data 1.0.

{
  /**
   Use `peekRecord()` to retrieve a record by it's type and id if present in the
   store. If there's no record matching the type and id `null` will be
   returned.

   This method is synchronous.

   `getById` will be deprecated and renamed to `peekRecord`.

   @method peekRecord
   @param {String} type
   @param {String} id
   @return {DS.Model|null} record
   */
  peekRecord: function(type, id),

  /**
   Use `peekAll()` to retrieve all records present in the store, by type. 

   This method is synchronous.

   `all` will be deprecated and renamed to `peekAll`.

   @method peekAll
   @param {String} type
   @return {DS.RecordArray} records
   */
  peekAll: function(type),

  /**
   Use `findRecord()` to retrieve a record by it's type and id if present in the
   store.

   If the record is in the store the store will ask the adapter if the
   record is stale. If the record is stale then the store will ask the
   adapter to refetch the record. If the record is not stale then it
   will resolve with the cached record immediately.

   If there's no record matching the type and id the store will ask the
   adapter to fetch the requested record.

   This method is asynchronous.

   `find(type, id)` will be deprecated and replaced by `findRecord`.

   @method findRecord
   @param {String} type
   @param {String} id
   @param {Object} options
   @return {DS.PromiseObject} record
   */
  findRecord: function(type, id, options),

  /**
   Use `findAll()` to retrieve all the records for a given type.

   Before asking the adapter to find all the records the store will
   first ask the adapter if the existing records it knows about are
   stale. If they are stale then findAll will wait for the adapter to
   find the records. Otherwise it will resolve immediately with all of
   the known records in the store.

   This method is asynchronous.

   `find(type)` will be deprecated and replaced by `findAll`.

   @method findAll
   @param {String} type
   @param {Object} options
   @return {DS.RecordArray} records
   */
  findAll: function(type, options),

  /**
   Use `fetchQuery()` to ask the adapter to fetch all records by type
   that match a query object. This method will always fetch new
   records and never resolve with cached records from the store.

   This method is asynchronous.

   `findQuery(type, query)` will be deprecated and replaced by `query`.

   @method query
   @param {String} type
   @param {Object} query
   @param {Object} options
   @return {DS.RecordArray} records
   */
  query: function(type, query, options),

  /**
   Use `queryRecord()` to ask the adapter to fetch all records by type that match a query object and
   it resolves with the first record returned.

   This method is asynchronous.

   @method queryRecord
   @param {String} type
   @param {Object} query
   @param {Object} options
   @return {DS.PromiseObject} record
   */
  queryRecord: function(type, query, options),
}
multiplicity local data fetch if not local; check for stale if local fetch always
single object store.peekRecord store.findRecord store.findRecord('type', 1, {reload: true})
collection store.peekAll store.findAll store.findAll('type', {reload: true})
single object query Out of scope for 1.0 Out of scope for 1.0 store.queryRecord
collection query Out of scope for 1.0 Out of scope for 1.0 store.query

@cibernox

cibernox commented Jun 6, 2015

Copy link
Copy Markdown
Contributor

I don't know if there is a meaning of peek I am not aware of, but sounds confusing to me. Did you mean pick?

@bmac

bmac commented Jun 6, 2015

Copy link
Copy Markdown
Member

I believe the idea is that peek (like in a stack) would not modify the internal state of the store. While find and query may have side effects on the store.

@cibernox

cibernox commented Jun 7, 2015

Copy link
Copy Markdown
Contributor

@bmac Since the store is not a stack of fifo collection the term peek seems inaccurate (à la isomorphic javascript).
I'd prefer something less imaginative like getRecord. The fact that getRecord doesn't performs a query while findRecord does is something that can be explained in ember-data 101 and I don't think anyone will find it problematic.

@igorT

igorT commented Jun 7, 2015

Copy link
Copy Markdown
Member

@cibernox we need a peek for sync access for relationships because get in that case does trigger a request.

@cibernox

cibernox commented Jun 7, 2015

Copy link
Copy Markdown
Contributor

I thought we were talking about methods in the store. If we're talking about assessing a collection without triggering a request peek seems better, but still...

@igorT

igorT commented Jun 7, 2015

Copy link
Copy Markdown
Member

@cibernox seems good to have consistency between the store and record methods?

@cibernox

cibernox commented Jun 8, 2015

Copy link
Copy Markdown
Contributor

@igorT absolutely, I was just wondering if take, pick or grab are more accurate since peek seems to be a term associated to stacks and queues. Probably just being picky.

@bmac

bmac commented Jun 8, 2015

Copy link
Copy Markdown
Member

@cibernox would you agree that peek in this context means basically the same thing it means with a stack?

Peek also has a history of being used with a cache API. Some examples:

@cibernox

cibernox commented Jun 8, 2015

Copy link
Copy Markdown
Contributor

@bmac Apparently the bias is only mine then

@jerel

jerel commented Jun 8, 2015

Copy link
Copy Markdown

peek also struck me as an uncommon naming choice before @cibernox brought it up.

By definition isn't peek the equivalent to shift except that it doesn't modify the sequence? (C#, Ruby, Wikipedia). I'd vote for grabRecord and grabAll or pickRecord and pickAll. I'm not wanting to bikeshed but neither do I want our naming to be more confusing than necessary to new developers.

@knownasilya

Copy link
Copy Markdown
Contributor

I think this one has pretty much been implemented by #61

@runspired

runspired commented Oct 31, 2018 •

Copy link
Copy Markdown
Contributor

I think we should close this in favor of an RFC improving the general management and flow of request/response fulfillment. Also as @knownasilya pointed out, much of this improvement has already happened.

@hjdivad hjdivad closed this Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs Champion T-ember-data RFCs that impact the ember-data library

Projects

None yet

Development

Successfully merging this pull request may close these issues.