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

Migrating from CRA to Vite #86

Merged
merged 13 commits into from
Oct 4, 2023
Merged

Migrating from CRA to Vite #86

merged 13 commits into from
Oct 4, 2023

Conversation

knightcube
Copy link
Contributor

Description

Followed the instructions to migrate from CRA to Vite - https://dev.to/henriquejensen/migrating-from-create-react-app-to-vite-a-quick-and-easy-guide-5e72

Fixes Issue 77 - #77

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!
👏 👏 👏
🥇

please fix the CI requirements before merge

package.json Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@knightcube knightcube mentioned this pull request Oct 3, 2023
README.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
playwright.config.ts Outdated Show resolved Hide resolved
Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

when running yarn start on your branch, it doesn't start on port 3000

tests/dashboard.spec.ts Outdated Show resolved Hide resolved
@NoamGaash
Copy link
Member

our tests suite uses HAR files as pre-recorded backend mocks.
the old regex matching in our tests was url: /api/.
with Webpack, it wasn't a problem, because Webpack creates a js bundle combining all source files.
with Vite, however, it uses the original files (at least in dev mode).
I changed the pattern to url: /^(?!.*\.(ts|js|mjs)$).*api/,, so now it will ignore script files, but it's not elegant.
In the future (and in general), I think it would be for the best if we test the production version (using the docker image) rather than running our tests on the development version.

@NoamGaash
Copy link
Member

trying to use localhost instead of 127.0.0.1, as it solved the same problem for someone else:
https://stackoverflow.com/questions/71943315/getting-error-connect-econnrefused-127-0-0-15432-in-github-actions

@NoamGaash NoamGaash merged commit 281a6a7 into hasadna:main Oct 4, 2023
4 checks passed
@knightcube
Copy link
Contributor Author

Yup. That is why I was using localhost earlier. I came across that same stack overflow link when I was debugging. Good to know that it works now!

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

Successfully merging this pull request may close these issues.

2 participants