invisible-college / statebus

All aboard the STATEBUS!!!
117 stars 5 forks source link

New subsets api #10

Closed toomim closed 10 years ago

toomim commented 10 years ago

This implements part of the new subset API. I'm assigning @mariusnita since he had the most critique of the last one.

Here's the spec: (copy/paste spec from https://github.com/TheInvisibles/Nerve/issues/9#issuecomment-45808396)

Requirements

Views must be able to

Cache must be able to

Server must be able to

Some views will need only partial data to be loaded. The views will communicate the subset of the data they depend upon by adding a parameter to the REST key:

    fetch("/proposal/34?subset=summary")

If a program only ever uses a single type of subset, it can skip the "=summary" part, and just say "?subset":

    fetch("/proposal/34?subset")

This only works when you don't have to distinguish multiple subset types.

Caching partial data

Next, we'll want to mark this information in the cache, so that when a new view needs the data, it knows whether it can use the cached version, or needs to get more data from the server. So we'll record:

    key = "/proposal/34?subset"

Or, if there are multiple subsets used in your app, then you specify a particular one with "?subset=

    key = "/proposal/34?subset=summary"

If multiple subsets have been loaded, the cache will keep track of them with:

    key = "/proposal/34?subset=summary,mobile"

If there's no subset specified at all, then the entire object is loaded:

    key = "/proposal/34"

The Union of all Subsets The cache wants to maintain a single object for all subsets. So it inserts objects into the cache by ignoring the '?__' part:

    cache['/proposal'] ==> {key: '/proposal?subset=summary' ...}

Over time, it will union them all together:

    key = "/proposal/34?subset=summary,extended"

Rendering loading indicators

I don't know the best technique yet, but here are 4 options:

A Coarse Distinction: You can write an if statement inside a component's render() function like this:

  render: function () {
             if (this.is_loaded()) {
                 ... render normal stuff ...
             } else {
                 ... render loading indicator ...
             }
          }

Getting Finer: You can subdivide your render by subsets with this:

  render: function () {
             if (this.is_loaded()) {
                 ... render normal stuff ...
             } else if (this.is_loaded('summary') {
                 ... render a partial loading indicator ...
             } else {
                 ... render loading indicator ...
             }
          }

The Dimension of Object: Each object also has a is_loaded(subset) method:

  render: function () {
             // Render the header
             if (this.props.proposal.is_loaded('summary')) {
                 ... render normal proposal header ...
             } else { header = render_spinner() }

             // Render the body
             if (this.props.proposal.is_loaded('all')) {
                 ... render normal proposal header ...
             } else { body = render_spinner() }
          }

The Finest Distinction: Or just see if fields exist. This might be the simplest technique:

render: function () {
    var header = section.header || spinner
    var page_number = section.page_number || spinner
    var body = section.body || spinner
    return <div>{{header}}</div><div>{{body}}</div><div>{{page_number}}</div>
}

component.is_loaded(subset) This function will check the component's required objects have been loaded, or a subset of them if subset is specified.

component.is_loaded(object, subset) This will check the objects' key to see if it has the required subset loaded yet. Returns false if not. The subset label is optional, and defaults to the entire object.

toomim commented 10 years ago

Let me be the first to question though... is this actually better:

  key = "/proposal/34?subset=summary"

than this:

  key = "/proposal/34?summary"

?

I kind of like how the latter is less verbose.

mariusnita commented 10 years ago

What do you think about these issues:

  1. that we use a key field in the object and a summary field in the url. Two fields in two different places. Can't we just use one field?
  2. that activeRest has two seemingly unrelated purposes: (a) to keep a cache of requested backend data and (b) to offer an interface for the "loaded state" of objects. Couldn't (a) and (b) be different libraries? Is it necessary to have them together?
  3. It's confusing whether ActiveRest actually cares that these are subsets. What if ?subset=summary and a "fully loaded" state have totally different fields? Why should activeRest care? It seems from its perspective, these could just be different objects (or different sets of properties) that get cached at the same key.
toomim commented 10 years ago

Ok, first off — we could just remove this whole subsets API. It isn't necessary. It's just there to make it more convenient to re-use data across pages that need different subsets of it. But if it's making the API more complicated and harder to understand then let's take it out!

I think going into lots of details debating this subsets API is what travis was referring to when he was mentioning premature abstraction. This subsets API isn't used very much yet, so we're probably doing premature refining.

Anyway, here is what I think about those particular issues:

(1.) I see this as just one field. (I replied offline to explain.)

(2.) I assume the interface to the "loaded state" you're talking about is the is_loaded() function. All that function does in the normal case is check the cache to see if the object is present. (I took out the actual "?loading" state in this pull request.) So we don't gain much by separating it unless we're talking about subsets. You could do the same thing by replacing is_loaded(object) with cache[object.key] != undefined.

If you specify a subset, then is_loaded() also checks that the subset is in the cached object's key. If we pulled that into a different library, all we'd be pulling out is this code:

            var cached_subsets = querystring_vals(cached.key, 'subset')

            // Check if it's loaded for an unlabeled '?subset'
            if (subset === 'subset')
                return cached_subsets.length == 0

            // Check if a labeled subset is present
            if (subset)
                return cached_subsets.indexOf(subset) != -1

If you're thinking about moving the whole subsets API into a separate library, then we'd also be changing the update_cache() function, separating out this logic:

                var old_subsets = querystring_vals(cached.key, 'subset')
                var new_subsets = querystring_vals(object.key, 'subset')
                var merged_subsets = array_union(old_subsets || [],
                                                 new_subsets || [])

                if (merged_subsets.length > 0)
                    // I'll have to generalize this later if we want
                    // it to support params in urls beyond 'subset'
                    // ... right now it wipes out all other params.
                    cache[key].key = key + '?subset=' + merged_subsets.join(',')

We would then need to give this second library a hook into the update_cache() function.

(3.) Let's ground this in an example. Imagine two pages that use different subsets of data Foo and Bar, where Foo is a subset of Bar. ActiveREST lets us load Foo after loading Bar without another request to the server. And we can load a subsection of Bar after loading Foo without another request to the server.

Now let's imagine that Foo has just a [title] field, and Bar has both [title, description] fields. ActiveREST will union these two fields together, so that when you fetch Foo after fetching Bar, you get [title, description] even though Foo only needs `[title]', and this is ok. You just have extra data.

If we didn't enforce strict subsets, you might imagine a Baz page that needs [user]. We could still union this with Foo and Bar, and get [user, title, description]. However, if Baz also has a title field, but with a different value, then it'll conflict with the existing title field and override it when loaded. This will mess up Foo and Bar.

So we need each of these to be subsets because we are unioning them all into the same object in the cache.

mariusnita commented 10 years ago

I guess my problem, possibly in general, is that I don't quite understand. I think maybe your presentations are too terse where I have to infer a lot of stuff in between and there are too many degrees of freedom there.

For example, I'm not sure if activeRest has to know about subtype relationships like "homepage<full". I think it would work if you just have a list of possible subsets, like "homepage, mobile, full" and ActiveRest is ignorant of any relationship there. So for example maybe "homepage < mobile" but the subsets currently loaded are "?mobile,full". And when you do a request "?homepage", ActiveRest queries the server again, because it doesn't know that all the fields needed by homepage are already present in "?mobile".

If all it is is a list, and you just append to the key whenever you load a new subset, and you can ask whether a particular subset is loaded, then I think I can understand.

toomim commented 10 years ago

If all it is is a list, and you just append to the key whenever you load a new subset, and you can ask whether a particular subset is loaded, then I think I can understand.

What you said there is true.

The only constraint is that each subset < full_object. There is no subset/superset relationship implied amongst the subsets themselves.