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

Is there a way to not include $__path key in the response payload #708

Closed
YesaDyuss opened this Issue Jan 18, 2016 · 11 comments

Comments

Projects
None yet
6 participants
@YesaDyuss

YesaDyuss commented Jan 18, 2016

When I am using falcor-htttp-datasource, the response payload contains $__path key everywhere. And I always have to filter it out. Is there a way to exclude it?

@jhusain

This comment has been minimized.

Show comment
Hide comment
@jhusain

jhusain Jan 18, 2016

Contributor

That key exists there to support the new deref function. It allows you to create a Model bound to an object anywhere in the message:

model.get("titlesList[0].name").then(v => {
var listItem = model.deref(v.json.list[0]);
return listItem.get("rating").then(p => log(p));
});

There's no way to hide it at the moment. I'm curious why you want to hide it. We considered adding in mode that would set enumerate to false, but I found it impacted performance.

Contributor

jhusain commented Jan 18, 2016

That key exists there to support the new deref function. It allows you to create a Model bound to an object anywhere in the message:

model.get("titlesList[0].name").then(v => {
var listItem = model.deref(v.json.list[0]);
return listItem.get("rating").then(p => log(p));
});

There's no way to hide it at the moment. I'm curious why you want to hide it. We considered adding in mode that would set enumerate to false, but I found it impacted performance.

@YesaDyuss

This comment has been minimized.

Show comment
Hide comment
@YesaDyuss

YesaDyuss Jan 18, 2016

@jhusain Is there an option to opt out of it. I just dont like all the filtering happenning in components,
especially in map functions

YesaDyuss commented Jan 18, 2016

@jhusain Is there an option to opt out of it. I just dont like all the filtering happenning in components,
especially in map functions

@michaelbpaulson

This comment has been minimized.

Show comment
Hide comment
@michaelbpaulson

michaelbpaulson Jan 19, 2016

Contributor

@YesaDyuss I am curious, why is this a problem? I would imagine this is only a problem if you are doing the following type of init'ing

var model = falcor.Model(...);

... later in code at a render function ...
var json = this.props.json;
return json.map(list => {
    return (
        <ListComponent json={list}></ListComponent>
    );
});

Then yes I could see this being a problem. Is that what is happening? if not, please elaborate why this is an issue.

Second, this should have nothing to do with falcor-http-datasource. This has everything to do with the falcor.Model. The http data source simply will take a list of args and call an end-point via http with the remotable falcor operation. Either the endpoint being called is returning a model payload (a no no, use a falcor-router or this is the data from the Model's output.

Let me know! But know that it will be a minimum 30% performance cost to use defineProperty to make $__path non-enumerable. Hence the reason why it has never been an option. But if this is truly a problem, then yes, we can consider adding one.

Contributor

michaelbpaulson commented Jan 19, 2016

@YesaDyuss I am curious, why is this a problem? I would imagine this is only a problem if you are doing the following type of init'ing

var model = falcor.Model(...);

... later in code at a render function ...
var json = this.props.json;
return json.map(list => {
    return (
        <ListComponent json={list}></ListComponent>
    );
});

Then yes I could see this being a problem. Is that what is happening? if not, please elaborate why this is an issue.

Second, this should have nothing to do with falcor-http-datasource. This has everything to do with the falcor.Model. The http data source simply will take a list of args and call an end-point via http with the remotable falcor operation. Either the endpoint being called is returning a model payload (a no no, use a falcor-router or this is the data from the Model's output.

Let me know! But know that it will be a minimum 30% performance cost to use defineProperty to make $__path non-enumerable. Hence the reason why it has never been an option. But if this is truly a problem, then yes, we can consider adding one.

@YesaDyuss

This comment has been minimized.

Show comment
Hide comment
@YesaDyuss

YesaDyuss Jan 19, 2016

Problem is roughly the same as you described: I pass _.values(json.movies) to ListComponent. And it is happening whenever I need to iterate over some collection. Now when upgraded to new falcor my components need to know about the existence of $__path and ignore them inside the component, or I need to filter them out when I am passing them to the component.

It is not a huge problem, but it is just annoying to change that in every place where I iterate over them and it loses some declarativeness, because web designers and new developers will be asking a question why are they using this dummy filter: _.values(json.movies).filter(movie => movie.title)..

To conclude I think $__path is related to falcor internals and it should not polute the output, that the user of the lib receives

Thanks for your extended response. The new update is great and I just want it to have as little friction as possible to the current workflow people already have

YesaDyuss commented Jan 19, 2016

Problem is roughly the same as you described: I pass _.values(json.movies) to ListComponent. And it is happening whenever I need to iterate over some collection. Now when upgraded to new falcor my components need to know about the existence of $__path and ignore them inside the component, or I need to filter them out when I am passing them to the component.

It is not a huge problem, but it is just annoying to change that in every place where I iterate over them and it loses some declarativeness, because web designers and new developers will be asking a question why are they using this dummy filter: _.values(json.movies).filter(movie => movie.title)..

To conclude I think $__path is related to falcor internals and it should not polute the output, that the user of the lib receives

Thanks for your extended response. The new update is great and I just want it to have as little friction as possible to the current workflow people already have

@trxcllnt

This comment has been minimized.

Show comment
Hide comment
@trxcllnt

trxcllnt Jan 19, 2016

Contributor

@jhusain @michaelbpaulson would be nice if we could have an optional jsonSelector(key: string, node: Object, refContainer?: Object) => Object in the options bag that the Model constructor accepts (just like errorSelector). It wouldn't be called if it isn't provided (so there'd be no perf hit), but would let people hook into the JSON that Falcor outputs. @YesaDyuss

Contributor

trxcllnt commented Jan 19, 2016

@jhusain @michaelbpaulson would be nice if we could have an optional jsonSelector(key: string, node: Object, refContainer?: Object) => Object in the options bag that the Model constructor accepts (just like errorSelector). It wouldn't be called if it isn't provided (so there'd be no perf hit), but would let people hook into the JSON that Falcor outputs. @YesaDyuss

@michaelbpaulson

This comment has been minimized.

Show comment
Hide comment
@michaelbpaulson

michaelbpaulson Jan 25, 2016

Contributor

I'll update this ticket with our full resolution to this problem.

Contributor

michaelbpaulson commented Jan 25, 2016

I'll update this ticket with our full resolution to this problem.

@michaelbpaulson

This comment has been minimized.

Show comment
Hide comment
@michaelbpaulson

michaelbpaulson Feb 1, 2016

Contributor

We are releasing master when we finish some documentation and one more small update to the migration guide. Then this ticket will be officially resolved in NPM. For now, we have fully decided on falcor.keys as the preferred solution.

Contributor

michaelbpaulson commented Feb 1, 2016

We are releasing master when we finish some documentation and one more small update to the migration guide. Then this ticket will be officially resolved in NPM. For now, we have fully decided on falcor.keys as the preferred solution.

@robwithhair

This comment has been minimized.

Show comment
Hide comment
@robwithhair

robwithhair Mar 29, 2016

Hi @michaelbpaulson. Is there a better solution on the way for this issue? I've got a really large codebase with lots of map() and similar. At the moment we've reverted to previous version of falcor but it's not a permanent fix obviously.

robwithhair commented Mar 29, 2016

Hi @michaelbpaulson. Is there a better solution on the way for this issue? I've got a really large codebase with lots of map() and similar. At the moment we've reverted to previous version of falcor but it's not a permanent fix obviously.

@robwithhair

This comment has been minimized.

Show comment
Hide comment
@robwithhair

robwithhair Mar 29, 2016

Also, the version on NPM seems to be 0.1.16 but still v0.1.15 in github. Is that correct? We're locked to 0.1.15 ATM to avoid this bug.

robwithhair commented Mar 29, 2016

Also, the version on NPM seems to be 0.1.16 but still v0.1.15 in github. Is that correct? We're locked to 0.1.15 ATM to avoid this bug.

@sdesai

This comment has been minimized.

Show comment
Hide comment
@sdesai

sdesai Mar 29, 2016

Contributor

Hey @robwithhair - it's the solution we've settled on for now, driven by the trade-offs below.

The options on the table were:

  1. Avoid adding the private property at all.

    This would be ideal. However, it was required to support deref(), while maintaining performance (IIRC, @jhusain or @michaelbpaulson may correct me here, but I think they mention it above).

  2. Have the property be non-enumerable.

    This would also have been functionally ideal also, but Object.defineProperty and other variations of this (leveraging the prototype chain etc.) all proved to be prohibitively expensive across platforms.

    An extension of this solution was to be able to switch between "clean output" OR "performant" modes, but the issue here was both the confusion the switch would cause (it's basically a "fastOrIterable" switch), as well as the leaning on such switches as a crutch whenever we needed to make decisions between "fast" or "clean", and ending up with multiple switches you could use to make things work faster, but have an associated set of usage caveats.

  3. Iteration util

    The motivation for ending up with this option, was that the open ended iteration use case seems to be the 20% case (the 80% being to map fields directly, or iterate over fixed ranges).

    Additionally if we work our way towards an alternate approach to a performant solution 1. or solution 2, as we progress, there's no immediate migration impact.

    I just caught this thread, but I think @trxcllnt's idea of being able to centrally register this support optionally, is a good option to put on the table. It has some of the same trade-offs as the central "switch" discussed in option 2 but has a more explicit opt-in from the user.

As implied above, I don't think we've ruled out pursuing the more ideal solutions if we can squeeze out the performance, but pursuing them may take a bit.


Regarding the 0.1.16 question - that seems to be a mistake.

We intended to publish backwards-compat breaking changes on a 1.x release (even though there are looser definitions for 0.x, and we'd still be in developer preview even with the 1.x) so that folks could use semver in builds reliably, and not continuously hit back-compat issues while evaluating the developer preview.

On cursory glance, it looks like we may have published 0.1.16 from the master branch by mistake, instead of the 0.x branch.

I'll be publishing 0.1.17 from the 0.x branch to "fix" 0.1.16, and publish a 1.0.0-rc from master [ I promised to do that last week, but got caught up with some other work. I'll try and get it out in a day or two ]

Contributor

sdesai commented Mar 29, 2016

Hey @robwithhair - it's the solution we've settled on for now, driven by the trade-offs below.

The options on the table were:

  1. Avoid adding the private property at all.

    This would be ideal. However, it was required to support deref(), while maintaining performance (IIRC, @jhusain or @michaelbpaulson may correct me here, but I think they mention it above).

  2. Have the property be non-enumerable.

    This would also have been functionally ideal also, but Object.defineProperty and other variations of this (leveraging the prototype chain etc.) all proved to be prohibitively expensive across platforms.

    An extension of this solution was to be able to switch between "clean output" OR "performant" modes, but the issue here was both the confusion the switch would cause (it's basically a "fastOrIterable" switch), as well as the leaning on such switches as a crutch whenever we needed to make decisions between "fast" or "clean", and ending up with multiple switches you could use to make things work faster, but have an associated set of usage caveats.

  3. Iteration util

    The motivation for ending up with this option, was that the open ended iteration use case seems to be the 20% case (the 80% being to map fields directly, or iterate over fixed ranges).

    Additionally if we work our way towards an alternate approach to a performant solution 1. or solution 2, as we progress, there's no immediate migration impact.

    I just caught this thread, but I think @trxcllnt's idea of being able to centrally register this support optionally, is a good option to put on the table. It has some of the same trade-offs as the central "switch" discussed in option 2 but has a more explicit opt-in from the user.

As implied above, I don't think we've ruled out pursuing the more ideal solutions if we can squeeze out the performance, but pursuing them may take a bit.


Regarding the 0.1.16 question - that seems to be a mistake.

We intended to publish backwards-compat breaking changes on a 1.x release (even though there are looser definitions for 0.x, and we'd still be in developer preview even with the 1.x) so that folks could use semver in builds reliably, and not continuously hit back-compat issues while evaluating the developer preview.

On cursory glance, it looks like we may have published 0.1.16 from the master branch by mistake, instead of the 0.x branch.

I'll be publishing 0.1.17 from the 0.x branch to "fix" 0.1.16, and publish a 1.0.0-rc from master [ I promised to do that last week, but got caught up with some other work. I'll try and get it out in a day or two ]

@robwithhair

This comment has been minimized.

Show comment
Hide comment
@robwithhair

robwithhair Mar 30, 2016

Thank you for the info. We should be able to return to tracking minor version updates after 0.1.17 is released.

robwithhair commented Mar 30, 2016

Thank you for the info. We should be able to return to tracking minor version updates after 0.1.17 is released.

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