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

New deref doesn't work when deref'ing to a reference. #559

Closed
trxcllnt opened this issue Sep 28, 2015 · 17 comments
Closed

New deref doesn't work when deref'ing to a reference. #559

trxcllnt opened this issue Sep 28, 2015 · 17 comments

Comments

@trxcllnt
Copy link
Contributor

model.deref(json.someKeyThatIsAReference) doesn't work because we don't attach the private __path or __key properties to ref values. We could do that, but then we'd need to clone the ref Array value every time. Another approach allowing deref to still accept a Path as an argument.

@ThePrimeagen
Copy link
Contributor

As of right now, with the new deref, no value-type can be bound to.

so if the output is like

{
  foo: {
    key: 'foo',
    parent: null
    bar: { $type: ref, value: ['to', 'ref'] }
  } 
}

then you can only deref to foo. Is this what you are asking about?

@falcor-build
Copy link
Contributor

The right option to address this issue is to special case to allow
deref-ing to references by detecting references as input and synchronously
creating a model bound to the reference path. The original hesitation here
is that there is no guarantee that the reference is not dangling - a
distinct possibility and an eventually consistent system. However given
that we have a way of signaling when a model is bound to invalid reference,
I'm inclined to allow binding specifically to references.

Imagine the following scenario: preload a series of references, and allow
free-form scrolling (showing gray box shots). A user clicks on an item
blind. You want to show it detailed view for the title, but don't want to
wait until some data has been downloaded for it. As a result you would like
to dereference immediately. Dangling references are so rare, and we have a
recourse for when derefing goes wrong. Under the circumstances I think we
should special case support references being passed to deref.

On Sep 28, 2015, at 8:53 AM, Michael Paulson notifications@github.com
wrote:

As of right now, with the new deref, no value-type can be bound to.

so if the output is like

{
foo: {
key: 'foo',
parent: null
bar: { $type: ref, value: ['to', 'ref'] }
}
}

then you can only deref to foo


Reply to this email directly or view it on GitHub
#559 (comment).

You received this message because you are subscribed to the Google Groups
"Falcor" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to falcor+unsubscribe@netflix.com.
To post to this group, send email to falcor@netflix.com.
To view this discussion on the web visit
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/143786218%40github.com
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/143786218%40github.com?utm_medium=email&utm_source=footer
.

@ThePrimeagen
Copy link
Contributor

To get this straight the feature to develop is the following.

if (derefValue.$type === $ref) {
    var ref = followReference(derefValue.value);
    return this._clone({_path: ref.path});
}
... do regular deref stuff here ...

followReference will resolve any references that follow references themselves.

@falcor-build
Copy link
Contributor

Yes. That's about the size of it.

JH

On Sep 30, 2015, at 9:24 AM, Michael Paulson notifications@github.com
wrote:

To get this straight the feature to develop is the following.

if (derefValue.$type === $ref) {
var ref = followReference(derefValue.value);
return this._clone({_path: ref.path});
}
... do regular deref stuff here ...

followReference will resolve any references that follow references
themselves.


Reply to this email directly or view it on GitHub
#559 (comment).

You received this message because you are subscribed to the Google Groups
"Falcor" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to falcor+unsubscribe@netflix.com.
To post to this group, send email to falcor@netflix.com.
To view this discussion on the web visit
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/144466529%40github.com
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/144466529%40github.com?utm_medium=email&utm_source=footer
.

@trxcllnt
Copy link
Contributor Author

@michaelbpaulson @falcor-build no that's not right. the value being passed in isn't the ref sentinel, but the value Array of a ref: { $type: 'ref', value: ['path', 'to', 'ref'] }. The boundJSONArg argument of deref will literally be an Array from the JSON, not a branch with the private __path, __key etc. values.

@ThePrimeagen
Copy link
Contributor

This could be a problem as it creates the old api, of which we were trying to avoid. Since I can just pass in any ole array, not whats actually in the secondary cache. But I don't know a good way around this issue.

@falcor-build
Copy link
Contributor

Why should we accept an Array? I want people to only bind to values
retrieved from the Model.

JH

On Sep 30, 2015, at 3:49 PM, Michael Paulson notifications@github.com
wrote:

This could be a problem as it creates the old api, of which we were trying
to avoid. Since I can just pass in any ole array, not whats actually in the
secondary cache. But I don't know a good way around this issue.


Reply to this email directly or view it on GitHub
#559 (comment).

You received this message because you are subscribed to the Google Groups
"Falcor" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to falcor+unsubscribe@netflix.com.
To post to this group, send email to falcor@netflix.com.
To view this discussion on the web visit
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/144567616%40github.com
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/144567616%40github.com?utm_medium=email&utm_source=footer
.

@ThePrimeagen
Copy link
Contributor

The problem is values that are retrieved in non boxValues mode. So the value passed in is not a type-value but just an array. Which then we are back to our old API.

@ThePrimeagen
Copy link
Contributor

After much discussion we will not support deref'ing an array. The major reason is the new deref is suppose to allow the secondary cache generated by the model to be derefable. This guarantees that the things being bound exist and by id, but allowing arrays gets us back to soft deref which is unstable/dangerous.

So I'll be closing this issue based on above.

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Oct 5, 2015

@michaelbpaulson tell @jhusain to stop protecting me from myself ;)

Seriously though, this is a breaking change. We used to be able to deref to a path that lands on a reference (e.g. model.deref('lolomo[0]')), and the reference would be fully resolved to its location in the cache. In fact, that behavior ("dereference") is literally where the the name comes from.

I'm sure there's some sort of metadata (private non-enumerable __ref key, etc.) we can attach to the Array value of a ref sentinel when we insert it so deref doesn't have to support all Arrays as valid input, just ones in the cache. The only alternative is using the unsupported _clone({ _path: myReference }) to get a model bound to my reference, and that's not entirely safe because my reference might land on a reference, etc.

@falcor-build
Copy link
Contributor

That is an interesting idea. It's worth considering, but it's very hard to
explain to users why some Arrays work and others don't. The main reason
behind the decision is that we can't come up with a use case. The method
exists to allow you to maintain consistency between what you have retrieved
previously and what you are displaying on the screen later on (don't want
to display an item in a list and a different item once clicked on). Near as
I can tell the only benefit to retrieving a reference and no data beyond it
is preloading references in a list so subsequent data retrievals are more
efficient. However as you cannot display a reference on screen, therefore
there is no need to maintain consistency if you attempt to retrieve
properties from the reference later on. You can just retrieve them from
whatever happens to be in the cache.

I'm curious what your use case is? When do you retrieve a reference and no
data beyond it?

JH

On Oct 4, 2015, at 8:58 PM, Paul Taylor notifications@github.com wrote:

@michaelbpaulson https://github.com/michaelbpaulson tell @jhusain
https://github.com/jhusain to stop protecting me from myself ;)

Seriously though, this is a breaking change. We used to be able to deref to
a path that lands on a reference (e.g. model.deref('lolomo[0]')), and the
reference would be fully resolved to its location in the cache. In fact,
that behavior is literally where the the function's name comes from "deref
erence".

I'm sure there's some sort of metadata (private non-enumerable __ref key,
etc.) we can attach to the Array value of a ref sentinel when we insert it
so deref doesn't have to support all Arrays as valid input, just ones in
the cache. The only alternative is using the unsupported _clone({ _path:
myReference }) to get a model bound to my reference, and that's not
entirely safe because my reference might land on a reference, etc.


Reply to this email directly or view it on GitHub
#559 (comment).

You received this message because you are subscribed to the Google Groups
"Falcor" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to falcor+unsubscribe@netflix.com.
To post to this group, send email to falcor@netflix.com.
To view this discussion on the web visit
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/145424690%40github.com
https://groups.google.com/a/netflix.com/d/msgid/falcor/Netflix/falcor/issues/559/145424690%40github.com?utm_medium=email&utm_source=footer
.

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Oct 6, 2015

@falcor-build @jhusain @michaelbpaulson I ran into this when I wrote a call function that computes references for an entity, but also fills in the data for those references. For example, model.call("getUserDetails", ["user-id-paul"]) might return the following JSON Graph Envelope from the server:

{ "jsonGraph": {
  "user": {
    "name": $atom("Paul"),
    "favorite-titles": {
      0: $ref(`titlesById[123]`),
      1: $ref(`titlesById[789]`),
      "length": $atom(2)
    },
    "recently-watched": {
      0: $ref(`titlesById[456]`),
      1: $ref(`titlesById[789]`),
      "length": $atom(2)
    }
  },
  "titlesById": {
    123: { "name": $atom("Pulp Fiction") },
    456: { "name": $atom("Danger 5") },
    789: { "name": $atom("Unsealed Alien Files") }
  }
} }

The JSON result from the call will look like this:

{ "json": {
  "user": {
    "name": "Paul",
    "favorite-titles": {
      0: ["titlesById", 123],
      1: ["titlesById", 789],
      "length": 2
    },
    "recently-watched": {
      0: ["titlesById", 456],
      1: ["titlesById", 789],
      "length": 2
    }
  },
  "titlesById": {
    123: { "name": "Pulp Fiction" },
    456: { "name": "Danger 5" },
    789: { "name": "Unsealed Alien Files" }
  }
} }

From here I'd like to use the respective lengths of user['favorite-titles'] and user['recently-watched'] to iterate through and create deref'd models for each of the list items, but can't since the value in the JSON is an Array, and deref doesn't support that anymore. My hack work-around is to use model._clone({ _path: itemRef.concat() }) to create deref'd models, but that suffers from the implicit aforementioned assumption that my reference doesn't land on another reference, etc.

@ThePrimeagen ThePrimeagen reopened this Oct 7, 2015
@ThePrimeagen
Copy link
Contributor

@trxcllnt it turns out, given your example, I think I have figured out what is happening here. This is actually an issue with the Router. Your [titlesById, 123, name] is produced via suffixes. Suffixes are generated off a call returned result references + provided suffixes. The path being reported to the jsonGraph Envelop is incorrect. It should be the path to the reference (in your case its [user, favorite-titles, 0] concated with the suffix [name]) not the reference path which would produce [user, favorite-titles, 0, name] as a path in the paths array. Therefore the JSON coming out of the call would not contain those references but the properly constructed json that is derefable.

https://github.com/Netflix/falcor-router/blob/master/src/run/call/runCallAction.js#L108

I'll file an issue with falcor-router.

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Oct 8, 2015

@michaelbpaulson unfortunately, I can't retrieve the data beyond the references using suffixes because in my case, the call function returns a large JSON Graph with more data than requested. If I used suffixes, the router would append the suffix to each of the references returned that don't point to titlesById.

Also, I should clarify: I'm using special branches in my forks of both falcor and falcor-router, which have some outstanding PR's/fixes already merged.

@ThePrimeagen
Copy link
Contributor

@trxcllnt Now I think I get it, here is the jsonGraph that is reported from you call

{ "jsonGraph": {
  "user": {
    "name": $atom("Paul"),
    "favorite-titles": {
      0: $ref(`titlesById[123]`),
      1: $ref(`titlesById[789]`),
      "length": $atom(2)
    },
    "recently-watched": {
      0: $ref(`titlesById[456]`),
      1: $ref(`titlesById[789]`),
      "length": $atom(2)
    }
  },
  "titlesById": {
    123: { "name": $atom("Pulp Fiction") },
    456: { "name": $atom("Danger 5") },
    789: { "name": $atom("Unsealed Alien Files") }
  }
},
"paths": [
    ['user', 'name'],
    ['user', 'favorite-titles', 'length'],
    ['user', 'favorite-titles', [0,1]],
    ['user', 'recently-watched', 'length'],
    ['user', 'recently-watched', [0,1]],
    ['titlesById', [123, 456, 789], 'name']
]  }

If I have guessed your paths correctly then I see the problem. Your call function is reporting the paths incorrectly. The paths should be reported as the following.

"paths": [
    ['user', 'name'],
    ['user', 'favorite-titles', 'length'],
    ['user', 'favorite-titles', [0,1], 'name'], // you know in your call  function what extra leaves you need
    ['user', 'recently-watched', 'length'],
    ['user', 'recently-watched', [0,1], 'name'] // same as above comment.
]  }

@trxcllnt
Copy link
Contributor Author

trxcllnt commented Oct 9, 2015

@michaelbpaulson

tl;dr: I could make it work without needing a deref change if falcor-router #128 gets merged in.

I looked back at the code that caused this, so I have a better example now:

import falcor from 'falcor';
import Router from 'falcor-router';

const $ref = falcor.ref;
const $atom = falcor.atom;
const $pv = falcor.pathValue;

const router = new Router([{
    /* search by customer name */
    route: ['search', 'titles-by-name'],
    call(callPath, [searchTerm]) {
        return fuzzySearchByTitleName(searchTerm)
            .flatMap(titles => titles.map(({ id, name, rating, relatedTitles }, titleIndex) => {

                const titleByIdPath = ['titles-by-id', id];
                const pathValues = [
                    $pathValue(['search', 'titles-by-name', searchTerm, titleIndex], $ref(titleByIdPath))
                ];

                pathValues.push(
                    $pathValue(titleByIdPath.concat('id'), $atom(id)),
                    $pathValue(titleByIdPath.concat('name'), $atom(name)),
                    $pathValue(titleByIdPath.concat('rating'), $atom(rating))
                );

                let relatedIndex = 0;
                const relatedLen = relatedTitles.length;

                if (relatedLen > 0) {
                    const relatedPath = titleByIdPath.concat('related-titles');
                    do {
                        const relatedTitle = relatedTitles[relatedIndex];
                        const relatedTitleId = relatedTitle.id;
                        pathValues.push($pathValue(
                            titleByIdPath.concat(relatedIndex),
                            $ref(['titles-by-id', relatedTitleId])
                        ));
                    } while (++relatedIndex < relatedLen);
                }

                return pathValues;
            })
            .concat($pathValue('search', 'titles-by-name', searchTerm, 'length', $atom(titles.length)))
        );
    }
}]);

const model = new falcor.Model({ dataSource: router });

function searchTitles(searchTerm) {
    model.call(['search', 'titles-by-name'], [searchTerm]).subscribe(({json}) => {
        const titleModels = [];
        const titlesByName = json.search['titles-by-name'];
        const titlesLen = titlesByName.length;
        let titlesIndex = -1;
        while (++titlesIndex < titlesLen) {
            const title = titlesByName[titlesIndex]; // title is an Array
            titleModels.push(model.deref(title)); // deref doesn't like Arrays :(
        }
    });
}

I could change this to append all the title information to the path prefix ['search', 'titles-by-name', searchTerm, titleIndex] instead of ['titles-by-id', id], but that's the solution that needs falcor-router #128.

@ThePrimeagen
Copy link
Contributor

@trxcllnt So the fix for you is that path values can follow references, which make sense for call preload cases. This could be easily accomplished. Netflix/falcor-router#142

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

No branches or pull requests

3 participants