Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve deref for better MVC Integration #501

Closed
jhusain opened this issue Aug 29, 2015 · 7 comments
Closed

Improve deref for better MVC Integration #501

jhusain opened this issue Aug 29, 2015 · 7 comments
Assignees

Comments

@jhusain
Copy link
Contributor

jhusain commented Aug 29, 2015

Currently deref has poor ergonomics for the following reasons:

  • it is async
  • it requires specifying a path to at least one value in the event that a round-trip is required to optimize the path (cannot retrieve branch nodes from a Falcor Server)
  • there is no guarantee that if you run a get, and asynchronously later run a deref, you will get the same object at a branch position. The JSON Graph could have changed after all.

The last issue is particularly problematic for UI integrations, because these integrations generally get a large block of data, distribute the JSON to component children, and also pass a deref'ed model to those Components that will later lazily load additional data. The issue is that component expansion is asynchronous in most MVC frameworks. This means that in between the get and deref call it is possible that the JSON Graph will have changed. As a result deref could be bound to different branch node than the one retrieved during get. This would cause a component to lazily load data from a different entity then it's eager load was retrieved from. At this puts the user interface into an inconsistent state.

At the moment the only solution is to deref all nodes immediately when the get call comes back, so that you have Models to dispense to Views in the event they need lazy loading. Obviously this is not efficient.

There is an elegant solution to the problem of deref:

To improve deref, we must first modify the get method to include a private “_path” property in the JSON whenever a reference is crossed. When a reference is not crossed, a reference to the parent node and the key at which to retrieve the current node at the parent node is set on the JSON branch node.

As an example, today get([“lolomo”,0,0,”item”,”name”]) returns this:

{
  genrelist: {
     0: {
         0: {
        item: {
          name: “Die Hard”
          }
         }
     }
  }
}

After this change, the same method would return this:

{
  genrelist: {
    _path: [“lolomos”,23],
     0: {                     <--|
         _path: [“lists”, 51],   |
         0: {                    |
           _parent: -------------|,
           _key: “0”, // key at which node can be found in parent
           item: {
             _path: [“videos”, 51],
             name: “Die Hard”
           }
      }
    }
  }
}

Now instead of deref accepting a deref Path and value paths, it accepts a branch node from the JSON.

model.
  get([“genrelist”,0,0,”item”,”name”]).
  then(response => {
    var lolomo = response.json.lolomo;
    var lolomoModel =   
      model.deref(response.json.lolomo);
  });

If deref finds the hidden _path parameter, it immediately creates a new Model bound to the optimized path. If it finds the _key parameter, it follows the _parent up until it finds a _path key. Along the way it builds up a path suffix using the _key fields, and when it discovers a _path it concatenates the suffix to it and uses the new path as the deref path.

Deref is now synchronous, never has to make a round trip, and requires no value paths to preload. The changes to "get" also requires no new allocations. The cost of the additional property sets in get should be offset by the dramatic reduction in value gets during deref. Furthermore avoiding the creation of an Observable is also expected to save quite a bit of time.

At first the only valid input for deref will be branch nodes created during a get operation. All other inputs will throw. It is up to the developer to guard against passing null, undefined, or any of the other value types to deref.The one exception is reference. If a reference is passed to deref, the reference's target path will become the Model's deref'ed path. If we assume no dangling references, we should maintain the invariant we have today: deref only creates a Model once it verifies that the reference target exists.

@jhusain
Copy link
Contributor Author

jhusain commented Aug 29, 2015

Forgot to mention that we will have to rev semver because this is a breaking change. I think it's the right decision. Old deref was really bad (my fault) and we're in preview mode. It's way too early to allow cruft to accululate on the API.

@sdesai please provide feedback if anything here is unclear.

@jhusain
Copy link
Contributor Author

jhusain commented Aug 29, 2015

@gdi2290 FYI

@PatrickJS
Copy link
Contributor

sweet, yeah this would help for Angular 2 integrations 👍

I wouldn't worry too much about the versioning during developer preview

semver.org

  • How should I deal with revisions in the 0.y.z initial development phase?

The simplest thing to do is start your initial development release at 0.1.0 and then increment the minor version for each subsequent release.

  • Doesn't this discourage rapid development and fast iteration?

Major version zero is all about rapid development. If you're changing the API every day you should either still be in version 0.y.z or on a separate development branch working on the next major version.

@falcor-build
Copy link
Contributor

To be clear, the field is not _path but <prefix>path. Because _path is a much more likely candidate for valid path than the prefix version.

@ggoodman
Copy link

Instead of devising a new JSON schema that augments a Model#get response with path metadata, why not have Model#get return an object:

  1. That understands the structure and location of the graph being requested and returned.
  2. That exposes a #deref(<jsonPath>) method that can be used to dereference a path within the scope of the graph it understands.
  3. That exposes a #toJSON() method that can be used to generate output consistent with what is already done?

As I work mostly in the Promised land, I'm not sure that my proposal would work very well in an Observable context.

@ewinslow
Copy link

ewinslow commented Sep 5, 2015

@ggoodman your idea sounds more like what I had in mind. Each model could take a basePath argument. Any call to get or getValue would prepend the basePath before firing of the request. Deref would just mean returning a new model with the given path appended to the current base path.

So deref looks like:

deref(subPath: Path): Model;

And you can do thing like:

model.deref('location').deref('address').getValue('formatted')

Which would be equivalent to:

model.getValue('location.address.formatted')

Am I understanding the purpose of deref correctly?

@ThePrimeagen
Copy link
Contributor

@ewinslow you do understand deref, but may not have all the subtle issues that come with deref. The issue with lazy deref (where the value passed into deref is not evaluated) does not seem to be an issue until you have a list of objects whose order changes on the UI. Imagine a grid of messages whose order changes on the UI when interacted with. To prevent any misalignment Model#get provides the correct deref information for the secondary cache it generates. This way the UI is dereference to ID that existed at the time of its creation.

@ggoodman the issue with your suggestion is performance. We have performance critical requirements as some of the devices we run on have low processing power. The performance overhead of creating these prototype style objects will simply cost to much. So a happy medium is a schema change where there are hidden objects on the output that hint deref on how to dereference the secondary cache so the UI remains consistent with what is being displayed.

This change was merged in #551

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants