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

Diagnosing failure after upgrading to v0.8.0 #76

Closed
colllin opened this issue Nov 13, 2015 · 19 comments
Closed

Diagnosing failure after upgrading to v0.8.0 #76

colllin opened this issue Nov 13, 2015 · 19 comments

Comments

@colllin
Copy link
Contributor

colllin commented Nov 13, 2015

Hey, How can I diagnose a silent failure after upgrading both react-router-relay and react-router?

I was already on react-router@1.0.0-rc3, but upgraded to Relay@0.5.0, react-router-relay@0.8.0, and react-router@1.0.0.

I switched to using <RelayRouter>, but now one of my routes doesn't correctly render its children.

I have something like...

ReactDOM.render(
    <RelayRouter
        history={createBrowserHistory()}
    >
        <Route path="/" component={AdminApp}>
            <IndexRoute component={CampaignList} queries={ViewerQueries} />
            <Route path="/campaigns/:campaignId" component={Campaign} queries={CampaignAndViewerQueries}>
                <IndexRoute component={CampaignMain} queries={CampaignAndViewerQueries} />
                <Route path="geometries" component={CampaignGeometries} queries={CampaignAndViewerQueries}>
                    <Route path="upload" component={CampaignGeometriesUpload} queries={CampaignQueries} />
                </Route>
            </Route>
        </Route>
    </RelayRouter>,
    document.getElementById('app-root')
);

And CampaignList renders correctly at the root (/), but at /campaigns/:id, Campaign doesn't render. My best guess is that it has something to do with having deeper nesting? Since CampaignList doesn't have any child routes but Campaign does.

@colllin colllin changed the title Diagnosing failure after upgrading to v0.5.0 Diagnosing failure after upgrading to v0.8.0 Nov 13, 2015
@taion
Copy link
Member

taion commented Nov 13, 2015

You're not getting an error or anything?

@colllin
Copy link
Contributor Author

colllin commented Nov 13, 2015

Nothing :/

@taion
Copy link
Member

taion commented Nov 13, 2015

Try setting e.g. a breakpoint in RelayRoutingContext.render. Is the route getting properly matched by the router?

@colllin
Copy link
Contributor Author

colllin commented Nov 14, 2015

How can I verify that? It looks like it's getting a campaignId param, so it's at least partly getting getting matched.

I've got a breakpoint in this function:

  RootComponent.prototype.render = function render() {
    return _react2['default'].createElement(_reactRelay2['default'].RootContainer, _extends({}, this.props, {
      Component: this._routeAggregator,
      renderFailure: this.renderFailure,
      renderFetched: this.renderFetched,
      renderLoading: this.renderLoading,
      route: this._routeAggregator.route
    }));
  };

And this._routeAggregator.route looks like:

{
  "name": "$$_aggregated-$$_route[2]_campaign-$$_route[2]_viewer-$$_route[3]_campaign-$$_route[3]_viewer",
  "queries": {

  },
  "params": {
    "campaignId": "Q2FtcGFpZ246MA=="
  }
}

@taion
Copy link
Member

taion commented Nov 14, 2015

Check the array of components. Also, what does the React devtool show as actually being rendered?

@pasviegas
Copy link

Also having problems after upgrading to 0.8 :(

There is nothing in the console and React devtools renders 'null' inside StaticContainer.

@taion
Copy link
Member

taion commented Nov 16, 2015

Try setting a breakpoint in RouteContainer and see why getData is not returning.

@colllin
Copy link
Contributor Author

colllin commented Nov 16, 2015

@taion The React devtool shows this:
image

It shows the CampaignList component when it should be Campaign.

Trying to find the array of components and getData now...

@colllin
Copy link
Contributor Author

colllin commented Nov 16, 2015

New information:

I was initially upgrading from:

"react-relay": "^0.3.2",
"react-router": "^1.0.0",
"react-router-relay": "^0.6.2",

to:

"react-relay": "^0.5.0",
"react-router": "^1.0.0",
"react-router-relay": "^0.8.0",

As an experiment, I tried upgrading to an intermediate step:

"react-relay": "^0.4.0",
"react-router": "^1.0.0",
"react-router-relay": "^0.7.0",

And I found the same bug there. Since I don't see any code changes between v0.6.2 and v0.7.0, it must be a bug in react-relay, right?

@colllin
Copy link
Contributor Author

colllin commented Nov 16, 2015

Ok, I was able to get it working with react-relay@0.4.0 and react-router-relay@0.7.0 by upgrading to babel-relay-plugin@0.3.0.

Unfortunately, I'm still stuck on getting react-relay@0.5.0 and react-router-relay@0.8.0 to work, even on the latest babel-relay-plugin@0.4.1.

@taion
Copy link
Member

taion commented Nov 16, 2015

Do you have a runnable example?

@colllin
Copy link
Contributor Author

colllin commented Nov 16, 2015

I made one. It seems to be related to switching queries between routes:

https://github.com/tomnod/react-router-relay-issue-76

@RaasAhsan
Copy link

I tried out your code, it seems that CampaignQueries is broke, If you take out the $campaignId parameter, it should work fine. The correct signature for the query function should be Component and then params. But if you try to stick any arguments in the function the whole thing breaks.

https://github.com/relay-tools/react-router-relay/blob/master/src/RouteAggregator.js#L58

@taion
Copy link
Member

taion commented Nov 16, 2015

Yup, per @Agrosis, you have the wrong signature for CampaignQueries. It should be either:

    campaign: () => Relay.QL`
        query {
            campaign(id: $campaignId)
        }
    `,

or

    campaign: (Component, params) => Relay.QL`
        query {
            campaign(id: $campaignId) {
                ${Component.getFragment('campaign', params)}
            }
        }
    `,

I had been doing something incorrect with ignoring the signature of the query function. It's not correct to specify a query function with arguments if you're not going to manually compose in the fragment.

@taion taion closed this as completed Nov 16, 2015
@colllin
Copy link
Contributor Author

colllin commented Nov 17, 2015

Totally fixed it. Thanks for your help @taion and @Agrosis . How do you feel about adding an error so that other people who make the same mistake have an easier time figuring it out? I can implement if you can direct.

Also, how/where did you learn this?

It's not correct to specify a query function with arguments if you're not going to manually compose in the fragment.

@taion
Copy link
Member

taion commented Nov 17, 2015

I can't - or rather it doesn't really make sense at that level for me to introspect into your query to figure out what it's doing. I'm giving you exactly what you'd get if you passed this query directly into upstream Relay, where it would also break in a similar way.

This is really just the syntax for writing Relay queries - you probably don't want to use the old syntax any more, but it used to be the only syntax before @devknoll added the shorthand.

@devknoll
Copy link
Contributor

Yeah, I don't think it was specifically mentioned anywhere, but it's been the documented way since relay@0.2.1. I think since that so closely followed release, it wasn't expected to affect a large number of people.

It looks like facebook/relay@658587e is relaxing the restriction here, which may make this even more confusing 😉

@taion
Copy link
Member

taion commented Nov 17, 2015

So for reference, this is the query:

    campaign: (campaignId) => Relay.QL`
        query {
            campaign(id: $campaignId)
        }
    `,

I don't believe this is valid under any version of Relay.

Previously we were being sloppy and just dropped all of the arguments entirely, which led to #72 and facebook/relay#585, which I fixed in c1092ed.

@devknoll
Copy link
Contributor

Got it.

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

5 participants