Skip to content

Conversation

stanbaker
Copy link
Contributor

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

@github-actions github-actions bot added ingestion PR or Issue related to the ingestion of metadata product PR or Issue related to the DataHub UI/UX smoke_test Contains changes related to smoke tests labels Nov 30, 2022
@stanbaker stanbaker marked this pull request as draft November 30, 2022 19:28
@github-actions
Copy link

Unit Test Results (build & test)

359 tests   - 262   359 ✔️  - 258   1m 4s ⏱️ - 14m 45s
  98 suites  -   59       0 💤  -     4 
  98 files    -   59       0 ±    0 

Results for commit bed2504. ± Comparison against base commit f69b469.

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
Copy link

Unit Test Results (metadata ingestion)

       8 files         8 suites   1h 0m 4s ⏱️
   765 tests    763 ✔️ 2 💤 0
1 532 runs  1 527 ✔️ 5 💤 0

Results for commit bed2504.

@anshbansal anshbansal added the community-contribution PR or Issue raised by member(s) of DataHub Community label Dec 1, 2022
@chriscollins3456
Copy link
Collaborator

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.

@stanbaker
Copy link
Contributor Author

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 stanbaker closed this Jan 4, 2023
@hsheth2 hsheth2 mentioned this pull request Dec 12, 2023
11 tasks
purnimagarg1 added a commit that referenced this pull request Sep 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata product PR or Issue related to the DataHub UI/UX smoke_test Contains changes related to smoke tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants