-
-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Relay support #662
Relay support #662
Conversation
…ate-react-app into relay-support merging upstream
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Re-merged with master. We now have eject support, nicer errors, and a great README. Note the environment variable is changed to REACT_APP_ GRAPHQL_URL |
I just tested this and the experience is really getting there – it's a big improvement over configuring @josephsavona suggested in #462 that the URL would be configured using the
I like that idea, but I see a couple of questions that need to be discussed:
|
Thanks for taking a look! Your comments align with things I have been thinking about. I thought about If we want to offer multiple ways of configuring the graphql url, then we should use graphql-config to do this. Let me know if we do. I can look into it.
Interesting idea. This is along the lines of where I was going with the
This part I am not sure about. I haven't looked much into the |
merging master branch into relay-support
I'm conflicted about this PR. On the one hand, we'd love to make it super easy to get started using Relay. On the other hand, this tool feels as if it should stay focused on the getting-started experience - helping new React developers get familiar with the ecosystem or start a small project. Instead of this PR, how about incorporating some of these changes directly into the Relay plugin? If we make Relay easier to configure in general, then we can revisit cc @gaearon |
Although I personally really needs this thing to work, I think including Relay, or Apollo, or any other non-tooling code in the core or creating a plugin system just to work around the opinionated architecture of create-react-app itself (eg. inaccessible babel and webpack configs) is against the main idea behind this project. I would rather see the core team focus on streamlining the process of updating an ejected of forked create-react-app. Then issues like adding Relay could be easily solved by adding a simple tutorial in the README. ps I would also consider talking to the Relay team and asking them drop the babel plugin, which is the main reason causing this hack. |
👍 I’m pretty sure we don’t want to have any Relay-specific files in the main |
@gaearon could you explain just a bit more what you mean by...
|
I appreciate the thoughtful comments from the group on this pull request. Honestly, I agree. This doesn't feel quite right in the core of create-react-app. Before we close the pull request, I'd like to get the group's thoughts on a few things: If we support relay through the babel plugin or through a separate package, how can we integrate the external package with Do we need to make the babel config and webpack config support some sort of hooks so packages can merge in some config like relay requires? @maxwell-oroark and I might maintain a fork called |
Thanks again for the PR. I’m going to close because, while we should ensure a good experience for Relay users, I don’t think adding a lot of code to CRA or a full-fledged plugin system makes sense at this point. Instead, for now, I would suggest forking Read #779 for more information. |
Hi @saintsjd @maxwell-oroark I've used your code and created fork of react-script for my personal usage. I also published it under |
Hey @svrcekmichal, can you open your fork for issues? And could you explain how to create the schema.json? Also I get a error of unexpected invocation at runtime. |
Hi @Valentin-Seehausen, I've opened issues. If you get error of unexpected invocation it means your schema.json does not match with queries used in relay. |
Hey, @svrcekmichal awesome work, thanks for that! Your fork works perfectly, but I don't need LESS and CSS-modules. And @saintsjd and @maxwell-oroark thank you, too! I didn't expect two Facebook products to be so incompatible. Or maybe there is something blocking Relay 2? :-) Anyway: When someone needs only relay and wants to stay unejected, one can also use my react-script as a basement for customization. It offers only relay support. https://github.com/Valentin-Seehausen/create-react-app |
Here is a first pass at support for relay in create-react-app from @maxwell-oroark and me. This is issue #462 . This pull request works for both local development server and for builds. I have tested following the CONTRIBUTING.md guide (both with
npm start
andnpm run create-react-app my-app
).For instructions on how to get relay going, refer to template/README.md.
There are some todos outstanding:
Please review and let me know your thoughts.