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

#11392 Migrate docs example sandboxes to use Vite instead of CRA #133

Merged
merged 5 commits into from
Aug 14, 2024

Conversation

hmousavin
Copy link
Contributor

Hello everyone!

Basically, I did 4 things:

  1. moved the "index.html" to the root of project, and removed the references to %PUBLIC_URL%
  2. removed the "react-scripts" from the package.json
  3. used vitest for testing, even though 2 of 3 projects didn't have tests, at all
  4. if there's a need to jsdom package, I added that package to project

Regards.

@hmousavin hmousavin requested a review from a team as a code owner July 27, 2024 09:42
@apollo-cla
Copy link

@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/

@jerelmiller
Copy link
Member

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!

@hmousavin
Copy link
Contributor Author

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.

@jerelmiller
Copy link
Member

@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 😄

@jerelmiller
Copy link
Member

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:

  • The change from JavaScript to TypeScript

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.

  • The change from Jest to Vitest

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!

@jerelmiller
Copy link
Member

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 🙂

@hmousavin
Copy link
Contributor Author

hmousavin commented Aug 1, 2024

Hello again @jerelmiller! 🙋‍♂️

Sure! I do my best to update the tests and make it work using Jest (not Vitest), ASAP.
And, by the way, I didn't think about the possible compatibility problems in Vitest! Thanks for the tip 😇

<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" />
Copy link
Member

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 🙂

Suggested change
<meta name="description" content="Web site created using create-react-app" />

@jerelmiller
Copy link
Member

Hey @hmousavin so sorry this fell out of my radar. Thanks so much for making these changes! I'll get this merged in.

@jerelmiller jerelmiller merged commit 4ede1dd into apollographql:main Aug 14, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

3 participants