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

reinitialization w/ vite #69

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

reinitialization w/ vite #69

wants to merge 27 commits into from

Conversation

mikellewade
Copy link

@mikellewade mikellewade commented Jan 24, 2024

Asana Ticket: https://app.asana.com/0/1205655776549324/1205770483276175/f

PR to port React Chatlog from yarn/CRA to npm/VIte:

Testing

create-react-app is no longer being maintained and therefore removed react-scripts from project and replaced with vitest. vitest is maintained by the vite team and offers benefits such as speed and pipelining configurations. Though we won't necessarily see the benefits of this with the scope of React chatlog, instructors (Mikelle, Ansel, and Ashley) decided that vitest would be the best route to take when weighed against configuring Jest testing for the project (Jest does not intrinsically know anything about JSX files). To run the testing command in terminal run npm run test and for VScode please install: https://marketplace.visualstudio.com/items?itemName=ZixuanChen.vitest-explorer

https://vitest.dev/guide/

Copy link
Contributor

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

The linter wasn't working for me out of the box. There are some lingering jest stuff that's gumming it up. I included the version of .eslintrc.js that I was able to get working, and the plugin that needs to be installed in the package.json. After that, a lint pass would be a good idea.

There are a few tickets for this project that we could include in this change as well (links in a comment)

The images should be updated for our new standards.

We should update the docker container and test it in learn to make sure that it works properly (see comment on test.sh).

.eslintrc.cjs Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
.eslintrc.cjs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
src/App.jsx Show resolved Hide resolved
src/App.test.jsx Outdated Show resolved Hide resolved
src/components/ChatEntry.jsx Outdated Show resolved Hide resolved
src/components/ChatEntry.test.jsx Outdated Show resolved Hide resolved
src/components/ChatLog.test.jsx Show resolved Hide resolved
Copy link
Contributor

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the other related tickets. I made a few phrasing suggestions. I'm also wondering whether we should leave some of those starting imports, even though they do cause linter errors at the start. Details in the comments.

README.md Outdated Show resolved Hide resolved
project-docs/setup.md Outdated Show resolved Hide resolved
project-docs/wave-01.md Outdated Show resolved Hide resolved
At this point, we can use the entire chat message data from the JSON file `messages.json` to test our code as you implement each component. We may choose to update the `ChatEntry` to use the `id` field in this wave.
At this point, we can use the entire chat message data from the JSON file `messages.json` to test our code as you implement each component. We may choose to update the `ChatEntry` to use the `id` field in this wave. At the end of this wave, all messages will be on the left side of the screen. For alternating left and right messages, please follow the instructions found in [optional enhancements](optional-enhancements.md).

![Wave 02 One Sided Messages](../images/react-chatlog-demo-one-sided-messages.png)
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

project-docs/wave-03.md Outdated Show resolved Hide resolved
import ChatEntry from './ChatEntry';
import { render, screen } from '@testing-library/react';
import '@testing-library/jest-dom';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is vitest compatible with this jest-dom testing library? Why did it change from the previous import '@testing-library/jest-dom/extend-expect';?

Copy link
Author

Choose a reason for hiding this comment

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

I had ran into the error Error: Missing "./extend-expect" specifier in "@testing-library/jest-dom" package and came across this as a solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. And that import is providing the toBeInTheDocument() method. And that neceissitated adding the vitest setup file to extend expect manually. Got it. Thanks. 👍

src/components/ChatLog.test.jsx Show resolved Hide resolved
package.json Show resolved Hide resolved
@@ -1,6 +1,4 @@
import React from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

The import React lines were causing linter warnings and are no longer necessary, so removing those is fine. 👍

For other imports in the project, even though they report linter warnings on the bare project, should we leave them in since they'll be needed later? Or maybe we should add reminders to the waves where they would be required for students to add them?

For the latter, I would say that we don't need to provide the explicit import line itself, but maybe something more along the lines of

Remember that the values we need for defining proptypes come from an external library and must be imported. We'll also need to be sure to import any other components used in each file.

Here especially, I'd be a little concerned that students wouldn't know to import the json file as we're doing, since we nevere really talk about that.

Copy link
Author

Choose a reason for hiding this comment

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

This crossed my as well, my initial thought was to leave them out since they would encounter errors in the console if they try to utilize them without importing. I figured it would be a good place for them to get that practice. I think your reminder is a good one! You want to add to every wave, correct?

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 prefer adding reminders to having the imports, that works for me too. In the required implementation, it looks like only waves 1 and 2 require adding imports. Wave 2 would need to also mention importing the json file (maybe say that the name used in the import will end up being the variable name we'll use to access the loaded data?) The optional enhancements might also require some imports depending on their implementation choices, so maybe a fairly broad statement about being sure to keep track of their imports as they work?

mikellewade and others added 4 commits May 6, 2024 15:19
Co-authored-by: Ansel Rognlie <[email protected]>
Co-authored-by: Ansel Rognlie <[email protected]>
Co-authored-by: Ansel Rognlie <[email protected]>
Co-authored-by: Ansel Rognlie <[email protected]>
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.

2 participants