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

Consider using different prop nams in RootContainer #785

Closed
Kureev opened this issue Feb 2, 2016 · 2 comments
Closed

Consider using different prop nams in RootContainer #785

Kureev opened this issue Feb 2, 2016 · 2 comments

Comments

@Kureev
Copy link

Kureev commented Feb 2, 2016

Hi there,

I'm pretty new to Relay, so correct me if I'm wrong, but it seems that Relay.RootContainer doesn't actually support vanilla Components, but a Relay.Containers instead.

Also, route is a pretty confusing name, does it make any sense to change it to something more suitable like query?

So, the idea is to use change a public interface to something like this:

<Relay.RootContainer
  ...
  Container={SomeRelayContainer}
  query={SomeRelayQuery} />

Does it makes any sense?

@wincent
Copy link
Contributor

wincent commented Feb 4, 2016

This is good feedback @Kureev. Thanks.

We are indeed moving away from the "route" terminology (see for example #456 and #503). If you look at RelayRenderer, which is a newer abstraction that Relay.RootContainer, you'll see that we've already moved towards using RelayQueryConfig shapes (passed in via a queryConfig prop) instead of routes, and we are shooting for Container rather than Component too (our inline docs actually say Container, but the implementation uses Component, so either the docs or the code are wrong; I'll see if I can fix this).

In the interests of not creating unnecessary compatibility-breaking changes, I think we're unlikely to make any major changes to the RootContainer API, so I am going to close this one for now, and I'll loop back with a link once I've looked into the Container vs Component issue on RelayRenderer.

Thanks once again for your input.

@wincent wincent closed this as completed Feb 4, 2016
@Kureev
Copy link
Author

Kureev commented Feb 4, 2016

Thanks for the nice feedback, @wincent!

ghost pushed a commit that referenced this issue Feb 4, 2016
Summary:
The code comments in `RelayRenderer` mention a `Container` prop,
suggesting that this is what we were intending to use at some point during the
design of the API, but as implemented the prop is actually `Component`.

As evidenced in this GitHub issue, this may cause confusion and there's an
opportunity for us to make things clearer by changing the name here:

#785

This diff continues to support `Component`, but issues a warning in `__DEV__`.
The code to handle the deprecation is ugly but fortunately can be short-lived.
As `RelayRenderer` isn't publicly documented yet, we don't have to keep the code
to support the deprecation around for long; I'm thinking after the next release
we can rip it out, with plenty of time to update internal callers.

Reviewed By: yungsters

Differential Revision: D2901998

fb-gh-sync-id: dfc7a44babf1f6a43749691d3430e92884c94454
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

2 participants