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

Upgrade-to-vite #119

Merged
merged 35 commits into from
Jun 11, 2024
Merged

Conversation

mayank1513
Copy link
Contributor

@mayank1513 mayank1513 commented Jun 6, 2024

  • Replace react-scripts with vite for better build and live reload speed
    • better dx
    • modern tooling
  • Set up vitest for unit testing
  • Updated dependencies to latest
  • Setup ESLint as per vite
  • Set up prettier for consistent code formatting
  • Refactor for better readability and maintainability
  • Fix lint issues

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2024

CLA assistant check
All committers have signed the CLA.

@lucas-koehler lucas-koehler self-requested a review June 7, 2024 12:49
Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @mayank1513 ,

thanks for this contribution ❤️ . Upgrading the seed app from CRA to ViteJS is certainly a good idea 👍

I have two remarks inline and noticed three further things:

  • Linting no longer works. The whole "eslintConfig" property in the package.json is invalid as the referenced configs are part of CRA and, thus, no longer present. This should be removed. With this, there is no more lint config. I suppose it would make sense to use a vitejs default lint setup for typescript. Could you add this?
  • For me the tests (via npm run test) cannot be executed because a dependency for jsdom is missing. We should add it as a dev dependency
  • Optional: The build complains that the deprecated CJS (common js) build is used. We could adapt the package.json and tsconfig.json to use ESM.

package.json Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@mayank1513
Copy link
Contributor Author

Hi @mayank1513 ,

thanks for this contribution ❤️ . Upgrading the seed app from CRA to ViteJS is certainly a good idea 👍

I have two remarks inline and noticed three further things:

* Linting no longer works. The whole `"eslintConfig"` property in the package.json is invalid as the referenced configs are part of CRA and, thus, no longer present. This should be removed. With this, there is no more lint config. I suppose it would make sense to use a vitejs default lint setup for typescript. Could you add this?

* For me the tests (via `npm run test`) cannot be executed because a dependency for `jsdom` is missing. We should add it as a dev dependency

* Optional: The build complains that the deprecated CJS (common js) build is used. We could adapt the package.json and tsconfig.json to use ESM.

Thank you for your observations. Probably I did not notice these as I was using yarn and pnpm locally. I have fixed the issues you observed.

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @mayank1513, thanks for the updates!

I only have a few minor comments inline. I also noticed that npm run cypress:ci does not seem to work and the tests fail. do the cypress tests work for you?

.tkb Outdated Show resolved Hide resolved
.vscode/extensions.json Outdated Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
src/components/schema.json Outdated Show resolved Hide resolved
src/components/uischema.json Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@mayank1513
Copy link
Contributor Author

mayank1513 commented Jun 10, 2024 via email

- remove .tkb ,,,
@mayank1513
Copy link
Contributor Author

Cypress is failing because this element does not exist in the DOM - [id="#/properties/recurrence"] > div

@mayank1513 mayank1513 force-pushed the upgrade-to-vite branch 3 times, most recently from c92c7f3 to 689d237 Compare June 10, 2024 17:37
@mayank1513
Copy link
Contributor Author

Fixed Cypress tests...

Copy link
Contributor

@lucas-koehler lucas-koehler left a comment

Choose a reason for hiding this comment

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

Hi @mayank1513 , thank you very much for the updates and the whole rework! Changes LGTM now ✨

I added commits for three small changes:

  • Fix one lint error in the cypress config
  • Execute CI in pull requests and add linting to CI
  • Regenerate the package-lock.json: We always do that for external/unknown contributors as a general security precaution.

@lucas-koehler lucas-koehler merged commit 92bea40 into eclipsesource:master Jun 11, 2024
6 checks passed
lucas-koehler added a commit that referenced this pull request Jun 11, 2024
- Replace react-scripts with vite for better build and live reload speed
- Set up vitest for unit testing
- Updated dependencies to latest
- Setup ESLint as per vite
- Set up prettier for consistent code formatting
- Refactor for better readability and maintainability
- Fix lint issues
- Fix cypress test
- Execute ci in pull requests and add linting to ci
---------

Co-authored-by: Lucas Koehler <[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.

3 participants