Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Add an alias option to graphql container #413

Merged
merged 3 commits into from
Jan 15, 2017

Conversation

SachaG
Copy link

@SachaG SachaG commented Jan 12, 2017

As explained in #354, your React component tree can get quite confusing when you have many nested Apollo(...) containers. This PR adds an alias option to the graphql HoC factory function:

options(ownProps) {
        return {
          alias: 'withCurrentUser',
        };
      },

Before:

https://d3vv6lp55qjaqc.cloudfront.net/items/0T3a352D3f2S0B0I1N1f/Screen%20Shot%202017-01-12%20at%2016.37.00.png?X-CloudApp-Visitor-Id=be6b2112ac44aa0ca58d2b5babe73626&v=26b9d644

After:

https://d3vv6lp55qjaqc.cloudfront.net/items/3E2W1J0O3H3U3N2j1l3V/Screen%20Shot%202017-01-12%20at%2017.01.40.png?X-CloudApp-Visitor-Id=be6b2112ac44aa0ca58d2b5babe73626&v=70e00ab8

TODO:

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change
  • If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

@SachaG
Copy link
Author

SachaG commented Jan 12, 2017

Also I wasn't sure where/how to write tests, so I thought I'd get feedback on the PR itself first and improve it later.

@tmeasday
Copy link
Contributor

Can you write a test that just renders a aliased wrapped component and then checks it's display name?

@@ -37,6 +37,7 @@ export declare interface MutationOptions {
optimisticResponse?: Object;
updateQueries?: MutationQueryReducersMap;
forceFetch?: boolean;
alias?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should be an OperationOption

@@ -156,7 +158,7 @@ export default function graphql(
const version = nextVersion++;
return function wrapWithApolloComponent(WrappedComponent) {

const graphQLDisplayName = `Apollo(${getDisplayName(WrappedComponent)})`;
const graphQLDisplayName = `${mapPropsToOptions({}).alias || `Apollo`}(${getDisplayName(WrappedComponent)})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you make it an OperationOption (like name), you won't need to do this janky thing.

@jbaxleyiii
Copy link
Contributor

This is awesome. I wonder if this can also be done with a babel transform which would be cool

@SachaG
Copy link
Author

SachaG commented Jan 13, 2017

@tmeasday is that what you meant?

alias: 'withFoo',
})
class Container extends React.Component<any, any> {
componentWillReceiveProps(props) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to do all this; it's sort of testing the query.metadata feature is working but there's a separate test for that (I assume).

Can you not just do

const instance = renderer.create(<Container>);
instance.displayName;

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get an error: Invariant Violation: Could not find "client" in the context of "withFoo(Container)". Wrap the root component in an <ApolloProvider>.

But if I wrap the component in <ApolloProvider>, I'm not sure how to access its displayName

@tmeasday tmeasday merged commit 978d9a2 into apollographql:master Jan 15, 2017
@tmeasday
Copy link
Contributor

Not sure what typescript is doing but Component.displayName is all we needed.

@SachaG
Copy link
Author

SachaG commented Jan 16, 2017

Nice! Btw I meant to submit it as a separate PR, but I also added a short note in the README on how to work with the package locally, I thought it might help others.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants