datahub icon indicating copy to clipboard operation
datahub copied to clipboard

perf(cicd/ui): migrate from CRA/Craco to Vite

Open stanbaker opened this issue 2 years ago • 2 comments

Overview

This migration from the current create-react-app build tooling is a pretty significant overhaul that touches quite a few areas. The primary reason for wanting to migrate is quicker dev server startup and production builds. It's also worth noting that the creator/lead maintainer of Webpack has stepped away from the project to work on a new build tool (Turbopack), causing some uncertainty for the long-term project roadmap as others step in to maintain.

Benchmarks

Dev server startup (yarn start)

  • Baseline: 52.74s
  • With Vite: 0.83s

Production build (yarn build)

  • Baseline: 96.95s
  • With Vite: 37.52s

All tests were performed using the latest from master (as of 11/30/22) on a 2021 M1 MBP (32GB). Average time was taken from 3 consecutive runs of the same test. Time spent generating graphql types (yarn generate) is excluded from comparison.

Major Changes

  • Local dev proxy server rewritten as Vite plugin (see devProxyPlugin.ts)
  • Frontend build pipeline no longer moves static assets from build/ to dist/. Vite outputs to dist by default.
  • Removal of manually prepending assets/ to certain urls. Vite should handle this automatically when hosting files between local dev and production builds.

What's left?

  • [ ] Fix Cypress startup
  • [ ] Fix remaining asset 404s (monaco editor library 3rd party JS and platform logos)

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

stanbaker avatar Nov 30 '22 19:11 stanbaker

Unit Test Results (build & test)

359 tests   - 262   359 :heavy_check_mark:  - 258   1m 4s :stopwatch: - 14m 45s   98 suites  -   59       0 :zzz:  -     4    98 files    -   59       0 :x: ±    0 

Results for commit bed25048. ± Comparison against base commit f69b4699.

This pull request removes 262 tests.
com.datahub.authentication.authenticator.AuthenticatorChainTest ‑ testAuthenticateFailure
com.datahub.authentication.authenticator.AuthenticatorChainTest ‑ testAuthenticateSuccess
com.datahub.authentication.authenticator.AuthenticatorChainTest ‑ testAuthenticateThrows
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testAuthenticateFailureMismatchingCredentials
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testAuthenticateFailureMissingAuthorizationHeader
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testAuthenticateFailureMissingBasicCredentials
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testAuthenticateSuccessDelegatedActor
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testAuthenticateSuccessNoDelegatedActor
com.datahub.authentication.authenticator.DataHubSystemAuthenticatorTest ‑ testInit
com.datahub.authentication.authenticator.DataHubTokenAuthenticatorTest ‑ testAuthenticateFailureInvalidToken
…

github-actions[bot] avatar Nov 30 '22 19:11 github-actions[bot]

Unit Test Results (metadata ingestion)

       8 files         8 suites   1h 0m 4s :stopwatch:    765 tests    763 :heavy_check_mark: 2 :zzz: 0 :x: 1 532 runs  1 527 :heavy_check_mark: 5 :zzz: 0 :x:

Results for commit bed25048.

github-actions[bot] avatar Nov 30 '22 19:11 github-actions[bot]

hey @stanbaker! Thanks so much for putting this together. I'm curious how important a change like this is for your team? In your description you mention the build times being the primary reason for the change, as well as some uncertainty about CRA's future leadership. How much of a blocker are build times for your team or is this just a nice to have?

The core team has some concerns about the risks of switching away from CRA due to all of the hidden complexities with single-page app builds and configurations that it takes care of for us. Also, while Vite certainly has a large following and usage, it's still somewhat less established than create-react-app (I feel less strongly about this than the risks of making the switch). Basically, we want to be very cautious and make sure we're doing the right thing here!

I think we're leaning towards holding off on this move until we have more signal that it's the right call, but we'd love to hear some more thoughts from you and your team.

chriscollins3456 avatar Dec 22 '22 14:12 chriscollins3456

hey @stanbaker! Thanks so much for putting this together. I'm curious how important a change like this is for your team? In your description you mention the build times being the primary reason for the change, as well as some uncertainty about CRA's future leadership. How much of a blocker are build times for your team or is this just a nice to have?

The core team has some concerns about the risks of switching away from CRA due to all of the hidden complexities with single-page app builds and configurations that it takes care of for us. Also, while Vite certainly has a large following and usage, it's still somewhat less established than create-react-app (I feel less strongly about this than the risks of making the switch). Basically, we want to be very cautious and make sure we're doing the right thing here!

I think we're leaning towards holding off on this move until we have more signal that it's the right call, but we'd love to hear some more thoughts from you and your team.

Hey @chriscollins3456, thanks for the feedback! This isn't a blocker/priority by any means, totally understand if you all would rather stick with what's working for now instead of taking some risk on entirely migrating build tooling! Closing this PR

stanbaker avatar Jan 04 '23 19:01 stanbaker