expose this.props.relay.getVarialbes() to component

[bchenSyd]

Hi there,
this PR is to address issue #1828 , #1984 , #1986

the key change is

packages\react-relay\modern\ReactRelayRefetchContainer.js

_getFragmentVariables = (): Variables => {
        return this._resolver.getVariables();
    }

because a selector always keep the correct/updated variables, so we should keep it as the source of true.

a unit test has been added to verify this.

thanks

[bchenSyd]

1. per our previous discussion, we might have an issue here;
   the fragmentVariables we passed to resolver is not pure; it is a super set of what should be used, and contains some fetch variables in most of the time.
   this can easily be fixed in another PR

2. not sure about the flow check failure as I didn't touch that file.

[bchenSyd]

@josephsavona @yuzhi can you guys please have a look?

[sibelius]

this looks good!

@kassens what do you think about it?

[alloy]

@josephsavona @kassens @wincent @yuzhi I’d also like to have this feature 🙏

[leebyron]

Thanks for this investigation!

Could you speak a bit more to the use case? I'd like to better understand the purpose of this new API (additions to documentation in the PR would be appreciated as well).

Part of the reason I ask is because I understand that this.context.relay.variables is one way of accessing variables

[bchenSyd]

thanks @leebyron for looking into it.
the unit test should speak for itself. Actually this PR was created 3 months and I believe there are many improvements have happened since then. If this.context.relay.variables can fit the purpose, then feel free to reject this PR
(we planned to migrate to relay-modern back in June but because we were using the legacy api setVariables so heavily and we couldn't found a way to detect what the current relay variables are, I created this PR. It wasn't approved and merged in so the mgmt team decided to park the relay-modern migration)

[alloy]

Oh didn’t realize that was a blessed way to access the variables. I was under the impression that access to React’s context should be abstracted away where possible. In that light, would it make sense to redo this PR by just exposing the context’s variables from the Relay prop?

[lukewlms]

I'm missing something here - I don't have a this.context.relay in any component (this.context exists, .relay is undefined). this.props.relay is available, but doesn't contain .variables.

How do we get access to this.context.relay?

[sibelius]

static contextTypes = {
   relay: PropTypes.shape({
     variables: PropsTypes.object,
   }),
},

try this commit fixes some missing variables 1ce348a

[bchenSyd]

@leebyron I've checked the latest code and can verify that this.context.relay.variables works well. It seems that this.props.relay.getVariables() way is not required anymore.

what else can we get from this.context.relay apart from the current fetch variables? I remember at the time when I created this PR, there were discussions about hidding this.context.relay and making this.props.getVariables a lazy implementation.

[bchenSyd]

merged with latest.

loop in @kassens as he is the author of having this.props.getVariables() in the first place
I still think it's worth it as it seems to be a quite common requirement to know the current query variables, and currently, we have to declare contextTypes to able to do it.

again. not urgent anymore but it's something nice to have in my view.

[bchenSyd]

I just found out that #1984 , #1986 have the same nature and confirm both can be resolved by this PR

[ekosz]

@leebyron @sibelius This solves a lot of bugs with the current relay version. Is there a reason there isn't more of rush to get this merged / released? I'm also having an issue where a pagination container's fragmentVariables are the default ones instead of the current ones. Which I assume is yet another related bug.

[Rodeoclash]

Accessing the context works great for finding variables that are defined on the QueryRenderer however I can't seem to see a way of accessing variables defined within @argumentDefinitions. The use case is that I'd like to use the count value defined in a defaultValue when calling relay.refetchConnection function.

That would allow one source of truth for determining the page size when fetching data (rather than having to define both in JS and GraphQL)

[MrSaints]

If anyone has issues accessing this.context.relay, you'll need to set the following in your class:

    static contextTypes = {
        relay: PropTypes.shape({
            variables: PropTypes.object,
        }),
    };

Also, import:

import PropTypes from "prop-types";

As per React > 15.

The last bit is pretty important, as a lot of answers online will suggest using React.PropTypes instead (which will not work - see: https://reactjs.org/docs/typechecking-with-proptypes.html).

[ekosz]

@kassens @jstejada Any consideration for this in 1.5.0? If not, what would need to be added to be considered? Looks like it already has a regression test. The merge conflicts are mostly about the addition of isLoading. I'm happy to resolve them and open a new PR if that's helpful.

[alloy]

@ekosz I at least would very much appreciate that and would test it in our apps.

[alloy]

I recently took a stab at updating this PR, but things changed significantly enough that this became more than dealing with mere merge conflicts. If anybody wants to take a stab at it or redo it, that would be much appreciated!

[ivosabev]

Any plans or merging this or exposing the variables in an alternative way?

[josephsavona]

Our general approach to this is to pass down query variables explicitly when we need them in components. Note that the local variables are already exposed by RefetchContainer and PaginationContainer - for example, this.props.relay.refetch(fragmentVariables => {...}) will let you decide what variables to refetch based on what the active variables are.

I'm going to close this PR since there's a workaround.