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

Make Compatible with Relay 2 #234

Closed
johntran opened this issue Apr 16, 2017 · 20 comments
Closed

Make Compatible with Relay 2 #234

johntran opened this issue Apr 16, 2017 · 20 comments

Comments

@johntran
Copy link

johntran commented Apr 16, 2017

Hi!

Using react-router-relay breaks in react-relay@1.0.1-alpha.2

It can be fixed if

  1. All instances of react-relay is replaced with react-relay/classic
  2. Install following package dependencies: react-relay@1.0.1-alpha.2, relay-compiler@1.0.0-alpha.2, babel-plugin-relay@1.0.1-alpha.2 (babel-plugin-relay published on npm as of April 15, 2017 won't work, will be explained later)
  3. Replace ./fixtures/babelRelayPlugin in react-router-relay/test/.babelrc with the following plugin and options [ "relay", { "compat": true, "schema": "./fixtures/schema.graphql" }] to fix tests.

You would have to generate a "schema.graphql" file from the "schema.js". Something like this in the updateSchema.js would work:

    import { introspectionQuery, printSchema } from 'graphql/utilities';
    const jsonFile = path.join(__dirname, 'schema.json');
    const graphQLFile = path.join(__dirname, 'schema.graphql')
    const json = await graphql(schema, introspectionQuery);
    fs.writeFileSync(jsonFile, JSON.stringify(json, null, 2));
    fs.writeFileSync(graphQLFile, printSchema(schema));

I could do a pull request, but I'm not sure how you would want the API to look like to support both Relay 1 and 2.

It is a bit early to start work on this, but it's good to keep in mind. The babel-plugin-relay package required to compile react-relay/classic is not published on npm yet, but you can download it from the 'facebook/relay' repo right now.

@taion
Copy link
Member

taion commented Apr 16, 2017

Thanks for mentioning this. Not quite sure how I want to tackle this yet – I think I want to move over the API entirely.

@chollier
Copy link

babel-plugin-relay is published you can install it with yarn add babel-plugin-relay@dev

@skosch
Copy link

skosch commented Apr 20, 2017

I wonder if this library will still be necessary at all? The docs seem to imply that QueryRenderer can be used independently from any routing implementation.

@taion
Copy link
Member

taion commented Apr 20, 2017

See discussion on facebook/relay#1628.

You can already use <Relay.Renderer> on its own – the problem is performance. The point of this library is to avoid the request waterfalls that you would get with the naive approach.

@skosch
Copy link

skosch commented Apr 20, 2017

Thank you @taion.

As long as every route corresponds to one container though (with nothing but dumb components) waterfalls shouldn't be a problem, correct?

@taion
Copy link
Member

taion commented Apr 20, 2017

If you nest <QueryContainer>s, you will have request waterfalls. That is exactly the problem.

@maletor
Copy link

maletor commented May 4, 2017

Does ReactRouter's bundling not help?

https://reacttraining.com/react-router/web/guides/code-splitting

@gauravtiwari
Copy link

Yes, if you have code splitting setup waterfall isn't an issue. Relay modern seems to work fine with React Router v4.

@taion
Copy link
Member

taion commented May 5, 2017

No, that's dangerously wrong.

The problem is that if you have nested routes, the naive approach of using a <QueryRenderer> (or <Relay.Renderer>) at each route component is going to lead inevitably to request waterfalls for the data.

Even if you're not doing any code splitting, this is still a problem. The nested <QueryRenderer>s don't start fetching until after their parents are done fetching.

zberkom added a commit to zberkom/react-router-relay that referenced this issue May 6, 2017
@adjourn
Copy link

adjourn commented May 20, 2017

Has anyone come up with any architecture or patterns?

Im starting to migrate to React Router v4 and Relay Modern but all I can think of right now is to use a single root level <QueryRenderer> which basically mimics React Router v3 and Relay Classic behaviour which doesn't seem like an upgrade.

@gauravtiwari
Copy link

@taion But if one doesn't nest this shouldn't be a problem right? Say each route correspond to one query renderer without any nesting and have route level async code-splitting.

@skosch
Copy link

skosch commented May 20, 2017

@gauravtiwari That's what I'm doing, and it works as expected. But @unirey is right, it's not much of an upgrade.

@skosch
Copy link

skosch commented May 20, 2017

I've actually switched to Apollo and haven't head any more headaches since. (It's likely though that Relay Modern has since ironed out a lot of the issues I had.)

@taion
Copy link
Member

taion commented May 20, 2017

If you want something that will work well with Relay Modern, take a look at Found + Found Relay. I haven't put the code up for Relay Modern integration yet, but I'll probably get something up next week.

The only thing I'm looking to accomplish is to allow using the equivalent of nested <QueryRenderer>s without hitting request waterfalls – exactly what this library does, but for Relay Modern. The answer for caching would remain "whatever you'd do without a router".

When I get a chance to check, I'll take a look at merging #235, but I'm not planning on doing further work here outside of maintaining support for Relay Classic in Relay v1.

There isn't really any good pattern for using React Router v4 with really any data fetching library without hitting request waterfall problems. If you want to get optimal data fetching performance with anything – Relay, Apollo, plain REST calls – and still colocate your query equivalents with your routes, then you don't want to be using React Router v4.

@lxwbr
Copy link

lxwbr commented May 24, 2017

If you want something that will work well with Relay Modern, take a look at Found + Found Relay.

@taion is there any short example to get one started with using Relay Modern and Found + Found Relay?

@taion
Copy link
Member

taion commented May 24, 2017

I don't have the code up yet – will do so soon. It should look pretty similar to what's here, except you'll specify one query instead of a query set.

@taion
Copy link
Member

taion commented May 30, 2017

Fixed in #237

Not going to support Relay Modern, but this will allow using Relay v1 at least

@taion taion closed this as completed May 30, 2017
@angelf
Copy link

angelf commented May 31, 2017

Please update the docs to describe that support is restricted to relay classic.

@taion
Copy link
Member

taion commented May 31, 2017

Good point

@taion taion reopened this May 31, 2017
@taion
Copy link
Member

taion commented Jun 1, 2017

done

@taion taion closed this as completed Jun 1, 2017
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

9 participants