chore: use pnpm to manage dependencies #12635

jacobwgillespie · 2022-11-05T12:19:31Z

Problem

Followup from #12520 (see there for more context), this PR replaces Yarn 1 with pnpm. Yarn 1 is semi-unmaintained and has caused issues in docker build.

Changes

This PR switches to use pnpm instead of yarn. The highlights are:

All yarn commands are updated to their pnpm equivalents
GitHub Actions workflows and the Dockerfile use pnpm now
Some additional dependencies have been added to package.json if they previously were only available due to hoisting.

How did you test this code?

Installed locally + ran docker build. Will need CI to run the tests.

Twixes

Looks good to me, but TypeScript is acting up, and I don't see why since nothing should be different with regard to typing. 🤔 Also something fishy going on with plugin server tests.

Twixes · 2022-11-07T08:46:25Z

frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx

-            addFilterDefaultOptions,
-        })
-        const { reportFunnelStepReordered } = useActions(eventUsageLogic)
+export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(function ActionFilter(


Why is this addition of function needed?

That's for the ESLint rule react/display-name - with the arrow function inside forwardRef, the wrapped component is unnamed.

Oh interesting, I thought we were already compliant with this rule. Looks like it's just been a while since this file was last edited and only now did ESLint catch this.

jacobwgillespie · 2022-11-07T12:51:02Z

TypeScript is acting up, and I don't see why since nothing should be different with regard to typing

Yeah... So this is the fun of migrating from Yarn 1 to pnpm (or a more strict Yarn) - Yarn 1 hoisted all modules to the root of node_modules, so you could potentially import a sub-dependency in your app without specifying it in package.json, and the behavior of tools that inspect node_modules can change. For instance, @types/* packages would be hoisted to the top, ESLint plugins would be hoisted, etc.

With pnpm or a more correct Yarn version, you can only import packages specified in package.json. This is way safer and more predictable, but when migrating you discover all the places that previously were accidentally working due to hoisting. 🙂

I'll keep watching the build, I think I'm getting closer to fixing all the TS issues.

Twixes

This is looking fantastic, well done! Ran this locally (bin/start) and everything's working.
The only remaining issue it those few failing Cypress tests – though I ran them on a clean setup locally with fully green results, so this might be some flakiness. Also not sure why Jest tests haven't ran in CI. Once the checks are green, we can merge this.

Twixes · 2022-11-07T15:19:41Z

patches/kea-forms@3.0.1.patch

@@ -0,0 +1,37 @@
+diff --git a/lib/types.d.ts b/lib/types.d.ts


CC @mariusandra

Ah, cool, yeah an upstream fix would be even better. 🙂

The issue here is that kea in 3.0.0-alpha accepted just two generic arguments for SelectorDefinition, but in the 3.x.x released version requires three. Looks like kea-forms assumes the alpha version's types.

Thanks for finding this! I released version 3.0.2 of kea-forms with the fix.

Twixes · 2022-11-07T15:28:38Z

frontend/src/scenes/insights/filters/ActionFilter/ActionFilter.tsx

-            addFilterDefaultOptions,
-        })
-        const { reportFunnelStepReordered } = useActions(eventUsageLogic)
+export const ActionFilter = React.forwardRef<HTMLDivElement, ActionFilterProps>(function ActionFilter(


Oh interesting, I thought we were already compliant with this rule. Looks like it's just been a while since this file was last edited and only now did ESLint catch this.

jacobwgillespie · 2022-11-07T18:53:43Z

this might be some flakiness. Also not sure why Jest tests haven't ran in CI. Once the checks are green, we can merge this.

Yeah, having done this before, the flakiness may very well be related to some module reshuffling, and after merge / deploy it's quite possible that there might be a followup issue to fix for the release. 🙂 Whenever this is merged, it would be good to watch for any errors, etc.

Twixes · 2022-11-08T16:23:19Z

.github/workflows/ci-frontend.yml

              # Split the test into 3 parts. To increase the number split change the denominator in `length / 3`
-              run: echo "test-chunks=$(yarn test --listTests --json | sed -n 3p | jq -cM '[_nwise(length / 3 | ceil)]')" >> $GITHUB_OUTPUT
+              run: echo "test-chunks=$(pnpm test -- --listTests --json | sed -n 3p | jq -cM '[_nwise(length / 3 | ceil)]')" >> $GITHUB_OUTPUT


Jest tests aren't running in CI. Looks like this should be sed -n 5p with pnpm + we might need to update ts-node, as I'm getting a "Non-string value passed to ts.resolveTypeReferenceDirective" error.
I would push this myself, but "Allow edits and access to secrets by maintainers" is unchecked on this PR.

Updated 👍

Allow edits and access to secrets by maintainers

Apparently GitHub doesn't support this if the repo and the fork are both located in organization accounts rather than personal accounts. 🤷 https://github.com/orgs/community/discussions/5634

Oh, that's very annoying. Now there are some "Jest encountered an unexpected token" problems in Frontend CI checks, and unfortunately I'm not able to push a fix either.

mariusandra

This looks good to me! The last changes I think we need are:

kea-forms can now be upgraded to a version that fixes the bug
multiple RUN commands in production.Dockerfile should be combined to one, in two places

Other than that, if there's no easy way to get CI working, we're just going to merge it in, and quickly fix things if any remaining issues arise. I think the stakes are low enough that we could get away with it.

jacobwgillespie · 2022-11-17T16:34:18Z

Okay, I've updated with changes from master and cleaned up the Dockerfile. I still need to fix Jest, which is currently not transpiling tests correctly (it still has import / export).

posthog-bot · 2022-11-25T07:30:56Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

mariusandra · 2022-11-25T08:51:45Z

We recently upgraded Jest and are running it now through esbuild, so that problem might have solved itself.

I'd be happy to merge it in, though we decided to wait until next week as we're just about to release a new version of PostHog... and just in case it's best to wait for after that :).

posthog-bot · 2022-12-05T07:31:11Z

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

jacobwgillespie · 2022-12-07T21:24:20Z

Closing in favor of #13190 🚀

* chore: use pnpm to manage dependencies * Fix CI errors * Don't report Docker image size for external PRs * Fix pnpm-lock.yaml formatting * Fix module versions * Ignore pnpm-lock.yaml * Upgrade Cypress action for pnpm support * Set up node and pnpm before Cypress * Fix typescript issues * Include patches directory in Dockerfile * Fix Jest tests in CI * Update lockfile * Update lockfile * Clean up Dockerfile * Update pnpm-lock.yaml to reflect current package.json files * remove yarn-error.log from .gitignore * formatting * update data exploration readme * type jest.config.ts * fix @react-hook issues for jest * fix react-syntax-highlighter issues for jest * fix jest issues from query-selector-shadow-dom * fix transform ignore patterns and undo previous fixes * add missing storybook peer dependencies * fix nullish coalescing operator for storybook * reorder storybook plugins * update editor-update-tsd warning to new npm script * use legacy ssl for chromatic / node 18 compatibility * use pnpm for visual regression testing workflow * use node 16 for chromatic * add @babel/plugin-proposal-nullish-coalescing-operator as direct dependency * try fix for plugin-server * cleanup * fix comment and warning * update more comments * update playwright dockerfile * update plugin source types * conditional image size reporting * revert react-native instructions * less restrictive pnpm verions * use ref component name in line with style guide Co-authored-by: Jacob Gillespie <jacobwgillespie@gmail.com>

charlescook-ph · 2022-12-14T15:14:27Z

Thank you so much @jacobwgillespie! Check your email for a small gift from us :)

jacobwgillespie · 2022-12-15T12:09:31Z

Hey, thank you! 🙌

jacobwgillespie mentioned this pull request Nov 5, 2022

chore: upgrade from Yarn 1 to Yarn 3 #12520

Closed

jacobwgillespie force-pushed the pnpm branch from cad3ae9 to 9d1d745 Compare November 6, 2022 18:15

jacobwgillespie added 2 commits November 6, 2022 18:26

chore: use pnpm to manage dependencies

4716f21

Fix CI errors

e58c954

jacobwgillespie force-pushed the pnpm branch from 9d1d745 to e58c954 Compare November 6, 2022 18:34

jacobwgillespie added 6 commits November 6, 2022 18:38

Don't report Docker image size for external PRs

1c32ceb

Fix pnpm-lock.yaml formatting

992d3e2

Fix module versions

21faf6a

Ignore pnpm-lock.yaml

e0e6ffc

Upgrade Cypress action for pnpm support

b4911bf

Set up node and pnpm before Cypress

5067ae6

fuziontech requested review from mariusandra, macobo, Twixes and timgl November 7, 2022 02:01

Twixes reviewed Nov 7, 2022

View reviewed changes

jacobwgillespie added 2 commits November 7, 2022 12:41

Fix typescript issues

e39b4e0

Include patches directory in Dockerfile

53eabad

guidoiaquinti mentioned this pull request Nov 7, 2022

chore(NodeJS): use Node.js v18 #12650

Merged

Twixes reviewed Nov 7, 2022

View reviewed changes

Twixes reviewed Nov 8, 2022

View reviewed changes

jacobwgillespie added 3 commits November 8, 2022 21:10

Fix Jest tests in CI

7118a15

Merge branch 'master' into pnpm

9cf0a09

Update lockfile

c79ed6d

mariusandra mentioned this pull request Nov 10, 2022

RFC: Software Engineering Language Policy at Posthog PostHog/meta#71

Open

mariusandra reviewed Nov 15, 2022

View reviewed changes

mariusandra approved these changes Nov 15, 2022

View reviewed changes

Merge branch 'master' into pnpm

8a7b54d

jacobwgillespie added 2 commits November 17, 2022 16:08

Update lockfile

bd3cc95

Clean up Dockerfile

bb2be35

posthog-bot added the stale label Nov 25, 2022

mariusandra removed the stale label Nov 25, 2022

posthog-bot added the stale label Dec 5, 2022

mariusandra removed the stale label Dec 6, 2022

thmsobrmlr mentioned this pull request Dec 7, 2022

chore: use pnpm to manage dependencies (closes #12635) #13190

Merged

jacobwgillespie closed this Dec 7, 2022

jacobwgillespie deleted the pnpm branch December 15, 2022 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: use pnpm to manage dependencies #12635

chore: use pnpm to manage dependencies #12635

jacobwgillespie commented Nov 5, 2022

Twixes left a comment

Twixes Nov 7, 2022

jacobwgillespie Nov 7, 2022

Twixes Nov 7, 2022

jacobwgillespie commented Nov 7, 2022

Twixes left a comment •

edited

Loading

Twixes Nov 7, 2022

jacobwgillespie Nov 7, 2022

mariusandra Nov 9, 2022

Twixes Nov 7, 2022

jacobwgillespie commented Nov 7, 2022

Twixes Nov 8, 2022

jacobwgillespie Nov 8, 2022

Twixes Nov 9, 2022

mariusandra left a comment

jacobwgillespie commented Nov 17, 2022

posthog-bot commented Nov 25, 2022

mariusandra commented Nov 25, 2022

posthog-bot commented Dec 5, 2022

jacobwgillespie commented Dec 7, 2022

charlescook-ph commented Dec 14, 2022

jacobwgillespie commented Dec 15, 2022

		@@ -0,0 +1,37 @@
		diff --git a/lib/types.d.ts b/lib/types.d.ts

chore: use pnpm to manage dependencies #12635

chore: use pnpm to manage dependencies #12635

Conversation

jacobwgillespie commented Nov 5, 2022

Problem

Changes

How did you test this code?

Twixes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobwgillespie commented Nov 7, 2022

Twixes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobwgillespie commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra left a comment

Choose a reason for hiding this comment

jacobwgillespie commented Nov 17, 2022

posthog-bot commented Nov 25, 2022

mariusandra commented Nov 25, 2022

posthog-bot commented Dec 5, 2022

jacobwgillespie commented Dec 7, 2022

charlescook-ph commented Dec 14, 2022

jacobwgillespie commented Dec 15, 2022

Twixes left a comment •

edited

Loading