🪟 🔧 Migrate from react-scripts to Vite #21421

timroes · 2023-01-13T20:02:44Z

What

Removed react-scripts (Create React App) as a build system and instead use Vite.

Why?

Because it's ⚡ faster, ⚙️ more configurable and way better 👷 maintained.

How

I've tried to document most files inline to give context why changes have happened. Some general remarks:

🔐 HTTPS required

Since Vite loads modules as individual files during development, we want to make sure to use HTTP2 to not run into parallel file loading limits of HTTP1. Thus HTTPS is required now also during development, i.e. the development page will now be available via https://localhost:3000 or https://127.0.0.1:3000. Since this will use self-signed certificates you need to ignore an "unsecure" warning in your browser. Due to the way Vite does caching it's also not necessary to disable HTTP caching during development and will be faster if this setting is off in your Dev Tools.

No code changes and further work

There has been no code change on actual source code for this. The only difference would be that Vite used import.meta.env instead of process.env. Since we still run tests via babel and Storybook via Webpack, I made sure process.env continues to work. That allows us to migrate Storybook and Jest in separate PRs, since both of them are quiet a bit of work to convert (and for Storybook we likely want to wait for the 7.0 release).

🔍 Code quality scans

Stylelint, ESLint and TypeScript runs in Vite (via the checker plugin) async in different workers. That means you might see warnings/errors coming from those in the UI with a bit of a delay, because the actual code already is delivered immediately, while the checks are still running. The new error overlay also doesn't pop up by default, but can be opened via a button on the bottom (Errors will still be printed to the terminal as well):

Testing

I've validated that ESLint (warnings as well as errors), Stylelint and TypeScript will fail the build with a non zero status code, to make sure they won't pass CI. Also tested Cloud mode and local development working, as well as OSS mode building proper docker images.

timroes · 2023-01-16T15:52:11Z

airbyte-webapp/src/views/common/StoreProvider.tsx

+    <ReactQueryDevtools
+      initialIsOpen={false}
+      position="bottom-right"
+      toggleButtonProps={{ style: { transform: "translateX(-75px)" } }}


ℹ️ Running on HTTPS we'll now also get the intercom banner in development, so I moved the react query devtool widget over to not overlap it.

timroes · 2023-01-16T16:00:35Z

airbyte-webapp/src/hooks/useSegment.tsx

@@ -1,3 +1,5 @@
+/// <reference types="@types/segment-analytics" />


ℹ️ Without types in the tsconfig.json (which was the case beforehand) all @types/... packages get automatically imported. That's not the case anymore now, so we'll reference the only missing one in this file explicitally.

Any reason to use triple-slash syntax over adding "@types/segment-analytics" to tsconfig.json? Seems like tsconfig.json might be a more centralized place to introduce this?

My main reason was just, that this way it's closer to "where it's used", i.e. if we were to remove Segment I feel it's clearer that we include the reference here. Technically it should work the same if we add it to the tsconfig.json. I don't have strong opinions either way though, so happy to have you make the call on this.

timroes · 2023-01-17T11:48:28Z

airbyte-webapp/build.gradle

@@ -106,16 +106,8 @@ task copyNginx(type: Copy) {
    into "build/docker/bin/nginx"
 }

-task stylelint(type: NpmTask) {


ℹ️ Stylelint now runs as part of the Vite (via the checker plugin). So it doesn't need a separate Gradle task anymore.

timroes · 2023-01-17T11:49:10Z

airbyte-webapp/package.json

+    "meow": "^9.0.0",
    "node-fetch": "^2.6.7",
+    "optionator": "^0.9.1",


ℹ️ Peer dependencies required by the styellint and eslint plugin for vite checker.

timroes · 2023-01-17T11:49:26Z

airbyte-webapp/package.json

    "orval": "^6.11.0-alpha.10",
    "prettier": "^2.6.2",
-    "react-scripts": "^5.0.1",


👋 bye, bye, you will for sure not be missed.

timroes · 2023-01-17T12:43:47Z

Successfull failure on eslint warning: https://github.com/airbytehq/airbyte/actions/runs/3939246270/jobs/6738917898

timroes · 2023-01-17T14:29:16Z

Currently blocked on #20887 . End to end tests are failing due to change in class names, which should be solved by that PR (and not requiring classNames for testing).

dizel852 · 2023-01-19T15:22:14Z

Unblocked - just merged #20887 🎉

flash1293 · 2023-01-20T13:00:08Z

I'm probably doing something wrong, but if that's the case it's easy to run into it. This is what I did:

Check out this branch
Run npm install
Run npm start
Go to https://localhost:3000/ (need to allow unsafe connection)
Fails like this:

timroes · 2023-01-20T13:08:49Z

Thanks for catching this. This is actually a valid thing I need to fix. Currently we set apiUrl to the same procotol in the defaultConfig, which we shouldn't since the local backend server does only listen to http. Will address that.

josephkmh · 2023-01-23T11:37:34Z

CORS issue is fixed in #21668, merged that in locally while I'm testing and request from https://localhost:3000 are working well again 👍

timroes · 2023-01-23T11:38:43Z

@flash1293 This should be solved now. You'll need to rebuild the server though (:airbyte-server:assemble) and restart your docker-compose to get it solved.

airbyte-webapp/src/config/defaultConfig.ts

josephkmh

Looks good to me! I tested locally in dev mode on https://localhost:3000 and everything worked smoothly. Same with a fresh build served via docker on http://localhost:8000

flash1293

LGTM; tested and seems to work fine.

I have one complaint, but I'm pretty sure that happened before, so no need to fix it on this PR: When defining a class in the scss module which is never referenced, it shows the expected error:

 ERROR(ESLint)  Unused classes found in BuilderField.module.scss: errorXXX, abc (css-modules/no-unused-class)
 FILE  /Users/joereuter/Clones/airbyte/airbyte-webapp/src/components/connectorBuilder/Builder/StreamReferenceField.tsx:10:8

     8 |
     9 | import { BuilderStream } from "../types";
  > 10 | import styles from "./BuilderField.module.scss";

However, when fixing the problem, this error doesn't go away - it sticks around until I restart the dev server.

timroes · 2023-01-23T17:05:26Z

However, when fixing the problem, this error doesn't go away - it sticks around until I restart the dev server.

This is more related to eslint and indeed currently present. You should though not need to actually kill the dev server, but you'll need to save the TS file that includes that SASS again. Since this rule is part of ESLint, it won't rerun if something in the SASS file changes, only when the TS file actually has a change.

timroes · 2023-01-23T18:44:09Z

The only check that failed, was tearing down the CI runner in the end, so treating this CI run as successful for merging.

timroes added 5 commits January 13, 2023 17:12

Migrate to Vite

0d8ffd9

Continue work on vite migration

71b2096

More environment fixes

146b855

Add CSP headers to dev server

507251a

Remove react-scripts

dfd92b6

octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 13, 2023

teallarson mentioned this pull request Jan 13, 2023

🪟 🔧 remove cyclic dependencies with cloud routes #21424

Merged

timroes mentioned this pull request Jan 13, 2023

Migrate to Vite #20499

Closed

Shim process.env

33214a7

timroes commented Jan 16, 2023

View reviewed changes

timroes added 2 commits January 16, 2023 16:56

Cleanup

c604532

Merge branch 'master' into tim/migrate-vite

ebb0fd4

timroes commented Jan 16, 2023

View reviewed changes

timroes added 3 commits January 17, 2023 11:04

Create ESLint failure for CI test

a6bd5be

Merge branch 'master' into tim/migrate-vite

4705784

create vite-plugins package

df8f641

timroes commented Jan 17, 2023

View reviewed changes

timroes added 5 commits January 17, 2023 13:20

Update nodeJS

7d25995

Make eslint warnings fail build

e2ef1ab

Merge branch 'master' into tim/migrate-vite

5b3c6f3

Remove trailing empty line in nvmrc

891c0de

Match package.json with nvmrc

bbb4e25

timroes added 2 commits January 17, 2023 13:44

Fix eslint test breakage

45f28a2

Revert node upgrade

c4dd5a2

timroes marked this pull request as ready for review January 17, 2023 13:08

timroes requested a review from a team as a code owner January 17, 2023 13:08

timroes added 2 commits January 19, 2023 17:15

Merge branch 'master' into tim/migrate-vite

d80c310

Merge branch 'master' into tim/migrate-vite

3332a83

timroes added 3 commits January 20, 2023 15:31

Remove setupProxy script

d17829b

Change default API endpoints to be http

7fabb5e

Merge branch 'master' into tim/migrate-vite

d2d940c

timroes commented Jan 23, 2023

View reviewed changes

airbyte-webapp/src/config/defaultConfig.ts Show resolved Hide resolved

josephkmh reviewed Jan 23, 2023

View reviewed changes

flash1293 approved these changes Jan 23, 2023

View reviewed changes

timroes added 2 commits January 23, 2023 18:05

Merge branch 'master' into tim/migrate-vite

b47fffe

Merge branch 'master' into tim/migrate-vite

dd7941e

krishnaglick approved these changes Jan 23, 2023

View reviewed changes

Merge branch 'master' into tim/migrate-vite

5b4e963

timroes merged commit 828b32a into master Jan 23, 2023

timroes deleted the tim/migrate-vite branch January 23, 2023 20:10

octavia-squidington-iii mentioned this pull request Jan 24, 2023

Bump Airbyte version from 0.40.28 to 0.40.29 #21767

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🔧 Migrate from react-scripts to Vite #21421

🪟 🔧 Migrate from react-scripts to Vite #21421

timroes commented Jan 13, 2023 •

edited

Loading

timroes Jan 16, 2023

timroes Jan 16, 2023

josephkmh Jan 23, 2023

timroes Jan 23, 2023

timroes Jan 17, 2023

timroes Jan 17, 2023

timroes Jan 17, 2023

timroes commented Jan 17, 2023

timroes commented Jan 17, 2023 •

edited

Loading

dizel852 commented Jan 19, 2023

flash1293 commented Jan 20, 2023 •

edited

Loading

timroes commented Jan 20, 2023

josephkmh commented Jan 23, 2023

timroes commented Jan 23, 2023

josephkmh left a comment

flash1293 left a comment

timroes commented Jan 23, 2023

timroes commented Jan 23, 2023

		@@ -1,3 +1,5 @@
		/// <reference types="@types/segment-analytics" />

🪟 🔧 Migrate from react-scripts to Vite #21421

🪟 🔧 Migrate from react-scripts to Vite #21421

Conversation

timroes commented Jan 13, 2023 • edited Loading

What

Why?

How

🔐 HTTPS required

No code changes and further work

🔍 Code quality scans

Testing

timroes Jan 16, 2023

Choose a reason for hiding this comment

timroes Jan 16, 2023

Choose a reason for hiding this comment

josephkmh Jan 23, 2023

Choose a reason for hiding this comment

timroes Jan 23, 2023

Choose a reason for hiding this comment

timroes Jan 17, 2023

Choose a reason for hiding this comment

timroes Jan 17, 2023

Choose a reason for hiding this comment

timroes Jan 17, 2023

Choose a reason for hiding this comment

timroes commented Jan 17, 2023

timroes commented Jan 17, 2023 • edited Loading

dizel852 commented Jan 19, 2023

flash1293 commented Jan 20, 2023 • edited Loading

timroes commented Jan 20, 2023

josephkmh commented Jan 23, 2023

timroes commented Jan 23, 2023

josephkmh left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

timroes commented Jan 23, 2023

timroes commented Jan 23, 2023

timroes commented Jan 13, 2023 •

edited

Loading

timroes commented Jan 17, 2023 •

edited

Loading

flash1293 commented Jan 20, 2023 •

edited

Loading