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

feat(ui): switch to vite and vitest #9451

Merged
merged 28 commits into from
Jan 3, 2024
Merged

Conversation

hsheth2
Copy link
Collaborator

@hsheth2 hsheth2 commented Dec 12, 2023

  • use vite instead of create-react-app
  • use vitest instead of jest
  • fix gradle caching for react build

The performance gains seem to be pretty similar to a previous PR (#6586) - huge gains for the dev server, and about a 3x speedup for production builds.

Compatibility

  • tl;dr: this should be a fully non-breaking change
  • In local development, API requests will still get forwarded to the datahub-frontend service
  • All image asset paths will continue to work
  • Existing REACT_APP_SOMETHING env vars will work
  • Theme customization using the json files should still work
  • Unit tests and cypress tests work the same without any major changes
  • The robots.txt file actually gets exposed correctly now

To Dos:

  • migrate to vite
  • migrate to vitest
  • yarn start working
  • eslint passing
  • unit tests passing
  • cypress tests passing

Other changes

  • Removes a bunch of npm dependencies that we no longer need
  • Cleans up some stale leftover code from our cypress test relocation
  • Removes unused graphql codegen stuff from datahub-frontend

Dev notes

  • The gradle fixes aren't perfect yet, but should be a marked improvement over the previous state. In particular, I still need to debug why yarn generate gets run too frequently. I'd also like to skip the yarnInstall step if the build is cacheable, but gradle doesn't really like that.
  • For the prosemirror issues, we followed the recommendations in Error: Looks like multiple versions of prosemirror-model were loaded ueberdosis/tiptap#577. The underlying issue was that multiple versions of certain prosemirror packages were getting loaded, causing conflicts. The resolutions thing fixes it for now, but we'll need to revisit and upgrade those package versions in the future.
  • The dev server and prod differ even less now. Currently, the only difference is the handling of the theme json, which enables hot reloading for theme changes in dev, but even that could be removed easily.
  • Image assets are both exposed to the react build (where they get hashes) and get statically copied to a specific location. This does mean that we have two copies of each image file, which does increase the size of the dist.
  • It probably makes sense to move the manifest.json file back into the assets directory, but we can do that in a future PR.
  • I removed everything related to miragejs. We previously used that library to mock our backend for the first iteration of our cypress tests. At some point we migrated our cypress tests to smoke-test and now use a live datahub backend instead of a mocked one, so everything related to miragejs was now stale.
  • Migrating from jest to vitest was pretty straightforward. The major changes were using vi instead of jest, and small tweaks to the way modules get mocked (specifically around default module exports and importActual).

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

@hsheth2 hsheth2 marked this pull request as draft December 12, 2023 20:51
@github-actions github-actions bot added the product PR or Issue related to the DataHub UI/UX label Dec 12, 2023
@hsheth2 hsheth2 marked this pull request as ready for review January 2, 2024 21:29
@hsheth2 hsheth2 changed the title feat(ui): switch to vite feat(ui): switch to vite and vitest Jan 3, 2024
Copy link
Collaborator

@chriscollins3456 chriscollins3456 left a comment

Choose a reason for hiding this comment

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

things are looking solid on my end. I pulled down and tested locally one more time and things are looking good!

@hsheth2 hsheth2 merged commit 4240578 into datahub-project:master Jan 3, 2024
41 checks passed
@hsheth2 hsheth2 deleted the react-vite branch January 3, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product PR or Issue related to the DataHub UI/UX
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants