🪟 🔧 Switch to `pnpm` for package managing #22053

timroes · 2023-01-28T17:16:34Z

What

Switches from using npm to pnpm for package managing in airbyte-webapp.

To use it locally you need to install the exact matching pnpm version:

# <version> must be the exact version from package.json > engines.pnpm
npm install -g pnpm@<version>
# Using it
pnpm i
pnpm start
# ...

Closes #21482

How

Replace the package-lock.json with the pnpm-lock.yaml file.
Switch Gradle tasks over to use pnpm instead of npm, also add an additional task that runs before pnpmInstall that validates we only have the pnpm-lock.yml file and no other lock file (to prevent accidental committing of a new package-lock.json file).
pnpm has a patch mechanism built in, so I could remove the nasty react-virtualize patch code from Vite and instead use the proper pnpm patch mechanism for it.
Couple of minor required ESLint and TS changes, that were caused by pnpm pulling in slightly newer versions.
Due to differences in some libraries version they also came with new SVGs, so I needed to udpate several snapshot tests.

timroes · 2023-01-28T18:15:33Z

airbyte-webapp/build.gradle

@@ -1,28 +1,46 @@
 plugins {
    id "base"
-    id "com.github.node-gradle.node" version "3.3.0"
+    id "com.github.node-gradle.node" version "3.4.0"


ℹ️ Required to have PnpmTask available.

timroes · 2023-01-28T18:17:18Z

airbyte-webapp/package.json

-    "start:cloud": "AB_ENV=${AB_ENV-frontend-dev} node -r ./scripts/environment.js -r ./scripts/dev-overwrites.js ./node_modules/.bin/vite",
-    "prebuild": "npm run generate-client",
+    "prestart": "pnpm run generate-client",
+    "start": "NODE_OPTIONS='-r ./scripts/dev-overwrites.js' vite",


ℹ️ With pnpm the node_modules/.bin/vite script is actually a wrapping Bash script not the js file of vite directly anymore, so we can't call it with node anymore, so using NODE_OPTIONS to pass down the -r parameter to the node process that will execute vite.

timroes · 2023-01-28T18:18:30Z

airbyte-webapp/src/components/connectorBuilder/useManifestToBuilderForm.ts

@@ -203,18 +200,18 @@ function manifestStreamSlicerToBuilder(
  }

  if (manifestStreamSlicer.type === "DatetimeStreamSlicer") {
-    const datetimeStreamSlicer = manifestStreamSlicer as DatetimeStreamSlicer;
+    const datetimeStreamSlicer = manifestStreamSlicer;


ℹ️ I believe pnpm pulled now in a newer version of orval which generates the types slightly better, so this isn't needed anymore.

timroes · 2023-01-28T18:19:17Z

airbyte-webapp/src/components/ui/Markdown/remarkAdmonitionsPlugin.ts

@@ -11,7 +10,7 @@ import styles from "./admonitions.module.scss";
 const SUPPORTED_ADMONITION_NAMES: Readonly<string[]> = ["note", "tip", "info", "caution", "warning", "danger"];
 const SUPPORTED_NODE_TYPES: Readonly<string[]> = ["textDirective", "leafDirective", "containerDirective"];

-export const remarkAdmonitionsPlugin: Plugin<[], Root> = () => (tree) => {
+export const remarkAdmonitionsPlugin: Plugin<[]> = () => (tree) => {


ℹ️ Typings works very fine with the Root so no need to pull in mdast (which is also the deprecated version of remark) for that.

timroes · 2023-01-28T18:19:55Z

airbyte-webapp/src/components/ui/NextTable/NextTable.tsx

@@ -26,7 +27,7 @@ export const NextTable = <TData,>({ columns, data, onClickRow, className }: Prop
            {headerGroup.headers.map((header) => (
              <th
                colSpan={header.colSpan}
-                className={classNames(styles.th, header.column.columnDef.meta?.thClassName)}
+                className={classNames(styles.th, (header.column.columnDef.meta as ColumnMeta | undefined)?.thClassName)}


ℹ️ The type overwrite via declare doesn't work with pnpm anymore (I don't fully understand why, yet), so moving it into an in place cast instead.

timroes · 2023-01-31T11:25:46Z

airbyte-webapp/.eslintrc.js

@@ -9,7 +9,7 @@ module.exports = {
    "plugin:jsx-a11y/recommended",
    "plugin:@airbyte/recommended",
  ],
-  plugins: ["react", "@typescript-eslint", "prettier", "unused-imports", "css-modules", "jsx-a11y", "@airbyte"],


ℹ️ This is with pnpm now conflicting with the react plugin already being pulled in by eslint-config-react-app which we extend above. Since that plugin pulls in the react plugin also no need for us to pull it in explicitally.

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

airbyte-webapp/.npmrc

@@ -1 +1,2 @@
 engine-strict=true
+enable-pre-post-scripts=true


ℹ️ Required for pnpm to execute the prestart etc scripts. It doesn't do so by default, but we're using the heavily for Orval compilation.

flash1293

Tested and everything still seems to work, lgtm.

Should we switch over the e2e test package as well for consistency reasons?

flash1293 · 2023-02-05T17:10:10Z

docs/contributing-to-airbyte/developing-locally.md

+- Install the `pnpm` package manager in the required version:
+
+```bash
+# <version> must be the exact version from airbyte-webapp/package.json > engines.pnpm


Not a blocker for this PR, but it looks like pnpm ships with a node version manager too, should we just use that one? One tool less

https://pnpm.io/cli/env

Didn't look into it in detail though

I have looked a bit into it, but currently didn't saw much of a benefit over keep using nvm (also we anyway need npm first to install pnpm, so we either need to guide to nvm or a local node installation in our docs).

You can install pnpm without npm, right? https://pnpm.io/installation

timroes · 2023-02-05T19:22:50Z

Should we switch over the e2e test package as well for consistency reasons?

Given we barely update anything in there and don't have much problems with npm there, I don't see a reason why... I also don't see a reason why not 🤷 (but would prefer doing it in a separate PR :D)

flash1293 · 2023-02-05T19:36:17Z

I don't see a reason why... I also don't see a reason why not

A user needs to make sure to call either npm or pnpm, depending on whether they are working in e2e or regular webapp (and keep both of them around and up to date)

timroes · 2023-02-05T19:40:41Z

A user needs to make sure to call either npm or pnpm, depending on whether they are working in e2e or regular webapp (and keep both of them around and up to date)

Problem is, we already have some other modules (the doc system) using yarn anyway, so I feel it's not that bad having npm for e2e tests, but I'll change it in a separate PR after this is merged.

flash1293 · 2023-02-05T19:43:13Z

Problem is, we already have some other modules (the doc system) using yarn anyway, so I feel it's not that bad having npm for e2e tests, but I'll change it in a separate PR after this is merged.

Good thing you mention this, that was a little annoying as well :) There's a lot of different technologies in this repo, would be nice to consolidate where possible.

But I don't think it's a big deal - as you say, it's not a big cost in day-to-day development

* master: Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164)

* master: (86 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...

* master: (24 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...

Tim Roes added 5 commits January 19, 2023 10:19

WIP

fd225c9

Merge branch 'master' into tim/pnpm

9b1d50b

WIP

35bf80e

Merge branch 'master' into tim/pnpm

0a164df

Adjust lockfile

63001be

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

timroes temporarily deployed to more-secrets January 28, 2023 17:18 — with GitHub Actions Inactive

Tim Roes added 3 commits January 28, 2023 18:34

Fix issues

2dab133

Adjust github flows

ff69510

More fixes

d361c06

timroes mentioned this pull request Jan 28, 2023

Update local webapp development instructions #21482

Closed

timroes temporarily deployed to more-secrets January 28, 2023 18:06 — with GitHub Actions Inactive

timroes temporarily deployed to more-secrets January 28, 2023 18:07 — with GitHub Actions Inactive

Adjust documentation

07b0b0b

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Jan 28, 2023

timroes temporarily deployed to more-secrets January 28, 2023 18:15 — with GitHub Actions Inactive

timroes commented Jan 28, 2023

View reviewed changes

Remove unnecessary tsconfig entry

b3649dc

timroes temporarily deployed to more-secrets January 28, 2023 18:35 — with GitHub Actions Inactive

Tim Roes added 2 commits January 31, 2023 12:03

Merge branch 'master' into tim/pnpm

2d1698b

Cleanup .eslintrc

ca66b4c

timroes commented Jan 31, 2023

View reviewed changes

timroes temporarily deployed to more-secrets February 2, 2023 18:42 — with GitHub Actions Inactive

timroes removed connectors/source/marketo connectors/source/mailchimp connectors/source/square labels Feb 2, 2023

Merge branch 'master' into tim/pnpm

5db4ad5

timroes temporarily deployed to more-secrets February 2, 2023 21:27 — with GitHub Actions Inactive

lmossman approved these changes Feb 2, 2023

View reviewed changes

flash1293 approved these changes Feb 5, 2023

View reviewed changes

Merge branch 'master' into tim/pnpm

bf1da86

timroes temporarily deployed to more-secrets February 6, 2023 13:13 — with GitHub Actions Inactive

timroes temporarily deployed to more-secrets February 6, 2023 13:59 — with GitHub Actions Inactive

timroes merged commit 86fbcdc into master Feb 6, 2023

timroes deleted the tim/pnpm branch February 6, 2023 15:29

github-actions bot mentioned this pull request Feb 7, 2023

Bump helm chart version reference to 0.43.28 #22489

Closed

This was referenced Feb 8, 2023

Bump Airbyte version from 0.40.32 to 0.40.33 #22578

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22689

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22839

Closed

🪟 🔧 Switch to pnpm for package managing #22053

🪟 🔧 Switch to pnpm for package managing #22053

Uh oh!

Conversation

timroes commented Jan 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timroes commented Feb 5, 2023

Uh oh!

flash1293 commented Feb 5, 2023

Uh oh!

timroes commented Feb 5, 2023

Uh oh!

flash1293 commented Feb 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

🪟 🔧 Switch to `pnpm` for package managing #22053

🪟 🔧 Switch to `pnpm` for package managing #22053

timroes commented Jan 28, 2023 •

edited

Loading

flash1293 commented Feb 5, 2023 •

edited

Loading