jameslaneconkling / falcor-local-datasource

Falcor DataSource interface exposing a local JSONGraph object
2 stars 0 forks source link

Issue with sample code #12

Open johndeighan opened 6 years ago

johndeighan commented 6 years ago

The sample code seems to work fine, but the problem is that if you ask for the number of todos before AND between the 2 calls, you'll get 2. However, if you ask for the number of todos AFTER the 2 calls, you'll get 4! Here is my code. Note that I used the code in the README file to set up the model, but used (my preferred) async/await to query the model. I did need to make these changes:

  1. Create and call a function main() so I could mark it as an async function, and therefore use await.
  2. Use require('falcor-local-datasource') to retrieve the LocalDatasource object
  3. Fixed an issue with the id's used, which I previously reported and thought was fixed, but it looks like it's still there (i.e. using 'id_0' and 'id_1' in one place, but 'id_1' and 'id_2' in another place)
const falcor = require('falcor');
// const LocalDatasource = require('../src/index');
const LocalDatasource = require('falcor-local-datasource');

async function main() {

const graph = {
  todos: {
    0: { $type: 'ref', value: ['todosById', 'id_0'] },
    1: { $type: 'ref', value: ['todosById', 'id_1'] },
    length: 2,
    add(graph, args) {
      const newTodoLabel = args[0];
      const todoCount = graph.todos.length;
      // NOTE: this is a pretty naive way to derive new ids.  a more robust approach might generate unique ids using
      // a specific node in the graph, or use a closure, or some other mechanism to yield unique incremental values
      const newId = `id_${todoCount}`;

      // return array of pathValues
      return [
        {
          path: ['todos', todoCount],
          value: { $type: 'ref', value: ['todosById', newId] }
        },
        {
          path: ['todosById', newId, 'label'],
          value: newTodoLabel
        },
        {
          path: ['todosById', newId, 'completed'],
          value: false
        },
        {
          path: ['todos', 'length'],
          value: todoCount + 1
        }
      ];
    }
  },
  todosById: {
    id_0: { label: 'tap dance', completed: false },
    id_1: { label: 'see above', completed: false }
  }
};

const model = new falcor.Model({ source: new LocalDatasource(graph) });

try {
    let n = await model.getValue('todos.length');
    console.log(`1. There are ${n} todos`);

    await model.call(['todos', 'add'], ['dupstep dance party']);

    n = await model.getValue('todos.length');
    console.log(`2. There are ${n} todos`);

    await model.call(['todos', 'add'], ['jumpstyle'],
                     [[['label', 'completed']]],
                     [['length']]);

    n = await model.getValue('todos.length');
    console.log(`3. There are ${n} todos`);
    }
catch (err) {
    console.error(err);
    }
} // main()

main();

Now, there is a way to fix this code, i.e. get 2, then 3, then 4 for the number of todos, but I don't think I should need to do this. The fix is to change the first "call" to:

await model.call(['todos', 'add'], ['dupstep dance party'], [], [['length']]); The way I understand the 4th argument is that it specifies values that I need in the code that uses model.call(), i.e. the code above, and I don't need it since I'm going to make another model.call() immediately after. However, the JavaScript function that implements the "add" operation returns 4 paths that should force the local cache to update its value for todos.length, and it doesn't seem to be doing that.

jameslaneconkling commented 6 years ago

OK, let me summarize to make sure I understand what you are seeing and expecting: The above code outputs:

> 1. There are 2 todos
> 1. There are 2 todos
> 1. There are 4 todos

And you are expecting:

> 1. There are 2 todos
> 1. There are 3 todos
> 1. There are 4 todos

If so, this is consistent w/ how Falcor works and is not a bug in local-datasource. Falcor only requests nodes that are specifically included in calls, and doesn't do any work to keep other nodes consistent in the case of side effects. The presumption is that calls are either atomic (don't have side effects), or manually do the work to make the rest of the local graph consistent.

In this case, oddly enough, the mutation to length is a side effect of adding an item to a list (oddly b/c we don't normally think mutating a js array as having a side effect, but lists in falcor are actually objects with indexes as ids and optionally a length key, e.g. { 0: {...}, 1: {...}, 2: {...}, length: 3 }).

So, there are generally two options to cover these cases:

johndeighan commented 6 years ago

I have to admit that I don't know how falcor-local-datasource works, so I figured that it was best to simply report the issue. However, I must admit also that I consider the fact that falcor-local-datasource does not support path invalidation to be a bug. At least in the sense that I can't use it and can't recommend it if that is the case. Maybe there's a good reason for that decision? Also, there's a difference between what is returned to my code and what is done to the local cache. If the JavaScript function that implements the call requests that a node in the JSON Graph be invalidated, it should be. Also, if the JavaScript function requests that the length be set to 3, it should be.

I don't mean to be harsh and I certainly don't understand why these things aren't done as the falcor documentation describes. Maybe I'll look at the falcor-local-datasource code and see if there is a fix.

On Tue, Jan 23, 2018 at 4:03 PM James Conkling notifications@github.com wrote:

OK, let me summarize to make sure I understand what you are seeing and expecting: The above code outputs:

  1. There are 2 todos
  2. There are 2 todos
  3. There are 4 todos

And you are expecting:

  1. There are 2 todos
  2. There are 3 todos
  3. There are 4 todos

If so, this is consistent w/ how Falcor works and is not a bug in local-datasource. Falcor only requests nodes that are specifically included in calls, and doesn't do any work to keep other nodes consistent in the case of side effects. The presumption is that calls are either atomic (don't have side effects), or manually do the work to make the rest of the local graph consistent.

In this case, oddly enough, the mutation to length is a side effect of adding an item to a list (oddly b/c we don't normally think mutating a js array as having a side effect, but lists in falcor are actually objects with indexes as ids and optionally a length key, e.g. { 0: {...}, 1: {...}, 2: {...}, length: 3 }).

So, there are generally two options to cover these cases:

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jameslaneconkling/falcor-local-datasource/issues/12#issuecomment-359929630, or mute the thread https://github.com/notifications/unsubscribe-auth/AE5M-f6C0dRN2MFDM-eYx_MV5VScuKWhks5tNkkmgaJpZM4RqRfq .

jameslaneconkling commented 6 years ago

yes, it is a bug (or feature), which is why it's tracked in the repo issues.

Also, there's a difference between what is returned to my code and what is done to the local cache

Nope, the behavior for returned data is 100% the same as what the HTTP datasource does. I guarantee if you set this up using a different datasource, you would experience the same behavior

if the JavaScript function requests that the length be set to 3, it should be.

Correct, and in fact it does. In your case, you are not asking to retrieve the updated length value, so there is no way for your model to know the length has updated. The data backing the datasource and the model cache are totally separate, which is by design (falcor's, not mine). If you don't request all data that might have been mutated by a call, there is literally no way for your model to be updated.

I don't mean to be harsh and I certainly don't understand why these things aren't done as the falcor documentation describes.

With the one caveat already mentioned that there is no interface for invalidation in the local-datasource function handlers, everything is done exactly as the falcor documentation describes.

jameslaneconkling commented 6 years ago

I'll take a look now at implementing invalidation--it shouldn't be too hard, just wasn't a priority when I wrote this a year ago.

johndeighan commented 6 years ago

I really do appreciate the work that has gone into this package and the fact that you're planning on working on implementing invalidation. However, I don't entirely agree with what you have said, so I'll try to make my point clearly and leave it at that.

The flow goes something like this:

my code ===> model (local) ===> datasource (usually, but not always remote) ===> JavaScript function

The model and the datasource both manage a JSON Graph object. The invariant that should always be true is that the JSON Graph object managed by the model is a correct subset of the JSON Graph managed by the datasource. By correct, I mean that for any path that exists in the model, the value at that location is the same as the value at the same path location in the datasource. If you don't agree that this should always be true, then I don't believe that you really understand falcor.

However, when this invariant is broken, the fault can be at several places. One of those places is the JavaScript function that is called to implement the call. That function must always arrange to let the datasource know what has changed. This can be done 2 distinct ways: 1) invalidate changed keys (recommended) or 2) set the value of changed keys to the new value. Either of those will result in the invariant above being maintained, either by pruning the model's JSON Graph object or by applying modifications. If the JavaScript functions are written correctly, and there is not a bug in the falcor code, nor in any intermediate code libraries used, there should be no way that user code can break this invariant condition. Of course, user code can do all kinds of bad things outside of that, but...

Unfortunately, your package is the first instance that I've seen anywhere of implementing the call interface (well, it's fortunate that it exists, but unfortunate that I can't find an explanation or example of how to do it otherwise). I've watched all the videos and read all of the online documentation multiple times. I must admit that it took me a long time to grok the parameters to the call method - the explanation isn't very good, but I know how difficult these things are to explain. I still think that Falcor is the right way to go, but it still needs some maturity. I'd really like to see a formal mathematical description of what it's doing, and it also doesn't seem that filtering a list of entities is addressed. But I'm really looking forward to seeing it mature.

On Tue, Jan 23, 2018 at 6:22 PM James Conkling notifications@github.com wrote:

yes, it is a bug (or feature), which is why it's tracked https://github.com/jameslaneconkling/falcor-local-datasource/issues/2 in the repo issues.

Also, there's a difference between what is returned to my code and what is done to the local cache

Nope, the behavior for returned data is 100% the same as what the HTTP datasource does. I guarantee if you set this up using a different datasource, you would experience the same behavior

if the JavaScript function requests that the length be set to 3, it should be.

Correct, and in fact it does. In your case, you are not asking to retrieve the updated length value, so there is no way for your model to know the length has updated. The data backing the datasource and the model cache are totally separate, which is by design (falcor's, not mine). If you don't request all data that might have been mutated by a call, there is literally no way for your model to be updated.

I don't mean to be harsh and I certainly don't understand why these things aren't done as the falcor documentation describes.

With the one caveat already mentioned that there is no interface for invalidation in the local-datasource function handlers, everything is done exactly as the falcor documentation describes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jameslaneconkling/falcor-local-datasource/issues/12#issuecomment-359965430, or mute the thread https://github.com/notifications/unsubscribe-auth/AE5M-eyEBVn_tQ40ljY6g3jw-alYoU25ks5tNmkFgaJpZM4RqRfq .

johndeighan commented 6 years ago

OK, not meaning to beat a dead horse, but I realized that I'd read something in the Falcor documentation to back up my view of things. In the Guide named "Model", under the heading "How Call Works", it says this (I'll paraphrase a bit to make it clear, but read it over if you think I'm not interpreting it correctly):

The datasource calls the JavaScript function. The JavaScript function returns a subset of the JSON Graph object held by the datasource "after the successful function completion" in a JSONGraphEnvelope. The JSONGraphEnvelope may also include a set of invalidated path sets (hence the need for the envelope). If refPaths and thisPaths were included in the original call, then the corresponding items are retrieved by the datasource from its JSON Graph and added to the JSON Graph subset that the function returned to it. Next, that completed JSON Graph subset is returned to the model and the model 1) removes any invalidated paths from its cache then 2) merges the JSON Graph subset that it received into its cache, then 3) returns the JSON Graph subset to the original caller.

I wish this were properly documented, but I believe that the JavaScript function is free to return the JSON Graph subset to the datasource in either of 2 ways: as a set of path values or as an actual JSON Graph object. In the case of your example, it's returned as a set of 4 paths:

todos[2] with value being a reference todosById['id_2'].label with value 'dupstep dance party' todosById['id_2'].completed with value false todos.length with value 3

From my interpretation of the docs, this JSON Graph subset should be included in the response sent from the datasource to the model, and should have been added to the model's JSON Graph subset, which it obviously was not since a separate and succeeding call to getValue() did not retrieve correctly because the model's JSON Graph subset has todos.length set to 2.

Of course, the recommended way to write JavaScript functions that are used by call() is to NOT set specific keys, but instead simply invalidate the changed keys. The caller can always either 1) request the data it needs, which will be fetched from the datasource when not found in the cache, or 2) include the needed keys in refPaths or thisPaths to avoid the extra round trip. But it does sound to me like some data should always be returned by model.call(). Now I understand your comment (in the code) that says "never runs". Personally, I believe that this is incorrect behavior, but in any case, i now understand why it currently works that way.

It is possible, I realize, that the Falcor documentation has changed since you last read it. It's also possible that one or both of us are interpreting things incorrectly. For now, I'm just trying to understand Falcor's and your package's inner workings and evaluate its utility for my work. Thanks for all your efforts.

jameslaneconkling commented 6 years ago

Huh, well unless I'm mistaken, there's a discrepancy between how get responses and call responses treat additional paths not included in the request. I had assumed that for both, additional paths are ignored and not added to the model cache. But, as you've pointed out, additional paths in a call response do in fact make it into the model cache. So yes, returning the additional ['people', 'length'] path in the call handler, even when it wasn't requested, should make it into the model cache. This is a bug.

This will take some extra time to fix, and I'm pretty busy at the moment. In the meantime, you can either use the refPaths or invalidation approaches (invalidation is now supported w/ 1.2.0), or perhaps a better approach is to use the Falcor Router directly in your client. The Router implements the DataSource interface, so even though it's typically deployed server side, there's no reason it can't be passed to the client model directly. This package was published when there was an issue using the Router client side, but now that it's fixed, it essentially supersedes this library's functionality. (I've been meaning to deprecate this package)

johndeighan commented 6 years ago

Thank you for all the work that you've put into this package. I think it's still useful, mainly because the Falcor docs suggest developing views without a back end, i.e. only a client with a local model cache, but neglect to say that you can't use call() with the Model object, though I'm not sure why not. Your suggestion of using a router client side makes sense, but I wonder if it's overkill when you aren't really doing any routing. Anyway, I have a path forward now - thanks again.

On Wed, Jan 24, 2018 at 12:50 AM James Conkling notifications@github.com wrote:

Huh, well unless I'm mistaken, there's a discrepancy between how get responses and call responses treat additional paths not included in the request. I had assumed that for both, additional paths are ignored and not added to the model cache. But, as you've pointed out, additional paths in a call response do in fact make it into the model cache. So yes, returning the additional ['people', 'length'] path in the call handler, even when it wasn't requested, should make it into the model cache. This is a bug.

This will take some extra time to fix, and I'm pretty busy at the moment. In the meantime, you can either use the refPaths or invalidation approaches (invalidation is now supported w/ 1.2.0), or perhaps a better approach is to use the Falcor Router https://www.npmjs.com/package/falcor-router directly in your client. The Router implements the DataSource interface, so even though it's typically deployed server side, there's no reason it can't be passed to the client model directly. This package was published when there was an issue using the Router client side, but now that it's fixed, it essentially supersedes this library's functionality. (I've been meaning to deprecate this package)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jameslaneconkling/falcor-local-datasource/issues/12#issuecomment-360028264, or mute the thread https://github.com/notifications/unsubscribe-auth/AE5M-cr_MS7Cy-N23IAbr3xLyT-RFHloks5tNsS6gaJpZM4RqRfq .