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 create-react-app to vite #305

Merged
merged 16 commits into from
Jun 13, 2023
Merged

Conversation

Pelsin
Copy link
Contributor

@Pelsin Pelsin commented Jun 6, 2023

This PR aims to migrate from create-react-app(CRA) to vite as CRA is no longer recommended. Since CRA has been stale for awhile we have a lot of deprecated dependencies:
Skärmavbild 2023-06-05 kl  19 13 03

Bundle before:
Skärmavbild 2023-06-05 kl  19 17 26
Bundle after:
Skärmavbild 2023-06-06 kl  01 47 43

Also changes files to .jsx which should make the bundling faster, it also helps us if we want to start migrating some of the files to TS further down the line, and helps the intelliSense that went bananas on some earlier files that where .js but had some TS in them.

The dev server has been converted to esm and also gets the .env which did not work before.

linting in CRA was quite opinionated and the one in this pr is using lots of defaults.
@FeralAI do you have some suggestions on rules that should be added?

I started out rewriting everything to TS but that was quite overwhelming(main...Pelsin:GP2040-CE:vite-typescript), so decided to make this PR and breaking it up best to my abilities.

Sorry for the wall of text and big PR!

@FeralAI
Copy link
Contributor

FeralAI commented Jun 8, 2023

This looks great @Pelsin! Tested it out and everything is working, along with the reduced bundle size 😄

Regarding the eslint rules, I ran npx eslint --ext .jsx,.js src/ and just about everything that's an error is something that should be addressed. We don't run the lint rules on build, so this PR won't break builds and doesn't need to address the errors.

The only rule I might want to add would be turning off no-extra-boolean-cast. There's a pattern for converting form values that uses this and I want to say it was for a reason, but don't recall what that was. I guess once this is merged we can test out removing/reworking those checks.

@Pelsin
Copy link
Contributor Author

Pelsin commented Jun 8, 2023

@FeralAI Nice, thanks for the quick review!

Added the no-extra-boolean-cast rule 👍
Makes sense with the linting tho, if we don't run it on build. After the boolean cast rule there is not many errors left so maybe we can add it to the build process soon :D

Resolved conflict so should be good to go then!

Copy link
Contributor

@arntsonl arntsonl left a comment

Choose a reason for hiding this comment

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

Looks solid! Will be cool to get our webpage up to a more modern framework.

@arntsonl arntsonl merged commit 62030d5 into OpenStickCommunity:main Jun 13, 2023
@Pelsin Pelsin deleted the vite branch June 13, 2023 15:24
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