-
Notifications
You must be signed in to change notification settings - Fork 46
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
#11392 Migrate docs example sandboxes to use Vite instead of CRA #133
Conversation
@hmousavin: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
Hey @hmousavin 👋 Thanks for the PR! I'll do my best to take a look at this PR within the next day or two. Appreciate the help here! |
You're so kind @jerelmiller ! Can you please, assign me some tasks? I want to get into the project and, don't know where to start! At least, some tips or suggestions would help me. ( Honestly, I read 11679 yesterday, but I don't have any idea how's that possible :| ) Thank you. |
@hmousavin if you're interested in contributing, take a look at our contribution doc which has some helpful guidance on contributing to Apollo Client and what we look for in pull requests! If you're looking to contribute code, feel free to take a look at some of our bug reports! Issues with a reproduction tend to be easiest to start with since you're able to see the issue in action. Perhaps something in here sounds interesting to you! Feel free to ping us if you'd like help identifying one to look at 🙂. Quick note about the contributing doc: we had some feedback that using the tutorial at the end of that doc isn't quite working. If you're wanting to try using your changes against an application, we suggest using yalc to handle those changes. This discord discussion has some helpful tips here from someone who just setup the client for local development recently. If you have additional questions, feel free to ping us on our discord server! Oh and on the note about apollographql/apollo-client#11679, I'm working on a full data masking feature right now and have a lot of the implementation details in my head. I've left these issues fairly terse for my own notes, but I don't expect anyone to understand how to approach these or contribute to these 😄 |
This mostly looks good. I'll provide a full review here soon, but I want to take the following changes to the team and see what we think about them:
While we write everything in TypeScript ourselves, I believe these were deliberately written in JavaScript to ensure someone without TypeScript experience could follow along. That being said, its so popular these days, this change might be ok. If we decide to leave this, we might want to consider updating the query docs page as well to ensure we have TypeScript examples since this page links out to the code in this repo.
Our existing docs on testing React components use Jest in the examples. We likely want to keep these Jest tests for now unless we want to rewrite that docs page to use vitest instead. Let me take these two questions back to the team and see what we think about each! I'll provide a full review once I get a chance to answer these questions. Thanks again for the contribution! Super helpful! |
Hey @hmousavin, thanks for your patience! The conversion to TypeScript in the examples is great, so let's keep that change around. As for the change from Jest to Vitest, would you mind updating the tests that are there to use Jest instead? While vitest may very well become the standard in the future, our docs are written with Jest for now and our testing tools aren't tested with vitest at the moment, so we can't guarantee compatibility. Once thats updated, I'll provide a full review so we can get this merged 🙂 |
Hello again @jerelmiller! 🙋♂️ Sure! I do my best to update the tests and make it work using Jest (not Vitest), ASAP. |
<link rel="icon" href="/favicon.ico" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
<meta name="theme-color" content="#000000" /> | ||
<meta name="description" content="Web site created using create-react-app" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're now using Vite, I think its safe to remove this 🙂
<meta name="description" content="Web site created using create-react-app" /> |
Hey @hmousavin so sorry this fell out of my radar. Thanks so much for making these changes! I'll get this merged in. |
Hello everyone!
Basically, I did 4 things:
Regards.