web: migrate from `yarn` to `pnpm` #46143

valerybugakov · 2023-01-05T03:32:44Z

Context

Bazel's rules_js rely on the pnpm package manager. To simplify the integration, we're migrating to pnpm from our current package manager — yarn. Another reason to migrate is that pnpm is cool and fast. 😉

Closes:

Breaking changes

If you used yarn commands directly, most of them should still work by replacing yarn with pnpm.
E.g., yarn build-ts —> pnpm build-ts. For more advanced commands, refer to pnpm documentation.

If you used only sg to interface with our npm scripts — nothing changes for you. All sg commands should work without changes.

Test plan

CI.
Local DX is intact.

Install pnpm locally with asdf install or npm i -g pnpm.

rm -rf node_modules ./client/*/node_modules
pnpm install

# Test your workflows!

…thub.com/sourcegraph/sourcegraph into contractors/SG-45990

feat: add client packages to workspaces dependencies

…ebpack loaders

sg-e2e-regression-test-bob · 2023-01-05T03:41:25Z

Bundle size report 📦

Initial size	Total size	Async size	Modules
-0.38% (-10.57 kb) 🔽	-0.03% (-4.02 kb)	0.06% (+6.55 kb)	0.42% (+3) 🔺

Look at the Statoscope report for a full comparison between the commits a765b31 and b277484 or learn more.

Open explanation

Initial size is the size of the initial bundle (the one that is loaded when you open the page)
Total size is the size of the initial bundle + all the async loaded chunks
Async size is the size of all the async loaded chunks
Modules is the number of modules in the initial bundle

valerybugakov · 2023-01-11T01:16:33Z

.npmrc

+# We have to hoist eslint packages to use them in the `.eslintrc` config.
+public-hoist-pattern[]=*eslint*


This is only required when Storybook packages are hoisted. We can remove both patterns after upgrading Storybook to v7.

sourcegraph-bot · 2023-01-11T01:18:38Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff b277484...a765b31.

Notify	File(s)
@bobheadxi	.github/workflows/licenses-check.yml .github/workflows/licenses-update.yml enterprise/dev/ci/internal/ci/cache_helpers.go enterprise/dev/ci/internal/ci/changed/diff.go enterprise/dev/ci/internal/ci/changed/files.go enterprise/dev/ci/internal/ci/operations.go
@efritz	doc/code_navigation/explanations/diagrams/generate.sh doc/code_navigation/how-to/index_a_typescript_and_javascript_repository.md doc/dev/background-information/codeintel/diagrams/generate.sh
@philipp-spiess	client/jetbrains/CONTRIBUTING.md client/jetbrains/package.json client/jetbrains/scripts/release.sh
@vdavid	client/jetbrains/CONTRIBUTING.md client/jetbrains/package.json client/jetbrains/scripts/release.sh

sourcegraph-bot · 2023-01-11T01:19:19Z

Codenotify: Notifying subscribers in OWNERS files for diff b277484...a765b31.

Notify	File(s)
@mrnugget	dev/sg/checks.go dev/sg/dependencies/helpers.go dev/sg/dependencies/mac.go dev/sg/dependencies/shared.go dev/sg/linters/linters.go dev/sg/linters/prettier.go dev/sg/linters/svg.go dev/sg/sg.config.example.yaml
@sourcegraph/code-exploration-devs	client/common/package.json client/http-client/package.json client/observability-client/package.json client/observability-server/package.json client/observability-server/src/honeycomb/clone-boards.ts client/observability-server/src/webBundleSize/index.ts client/wildcard/package.json client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.story.tsx client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.test.tsx
@sourcegraph/dev-experience	dev/sg/checks.go dev/sg/dependencies/helpers.go dev/sg/dependencies/mac.go dev/sg/dependencies/shared.go dev/sg/linters/linters.go dev/sg/linters/prettier.go dev/sg/linters/svg.go dev/sg/sg.config.example.yaml enterprise/dev/ci/internal/ci/cache_helpers.go enterprise/dev/ci/internal/ci/changed/diff.go enterprise/dev/ci/internal/ci/changed/files.go enterprise/dev/ci/internal/ci/operations.go
@sourcegraph/frontend-platform-devs	client/build-config/package.json client/build-config/src/paths.ts
@vovakulikov	client/wildcard/package.json client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.story.tsx client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.test.tsx

valerybugakov · 2023-01-11T04:56:29Z

Backend integration tests fail on main too 👀

philipp-spiess

🚀 🚀 🚀

I wonder what's causing the bundle size to degree but I AM DOWN FOR IT!

Let's get this merged sooner rather than later, the surface for conflicts seem huge 😅

I was wondering if we can install a custom yarn shim somehow that only does echo "Use pnpm instead" && exit 1 or so but I wouldn't know where to put it so that it is in everyone's PATH and takes precedence over the asdf installation one 🤔

philipp-spiess · 2023-01-11T10:09:04Z

.github/workflows/licenses-check.yml

+    - uses: pnpm/action-setup@v2
+      id: pnpm-install
+      with:
+        version: 7.24.2


To avoid hard coding the version in the github workflow, I've used this action before. Maybe we can reuse it here?

# The pnpm GitHub action does not support reading the version from # .tool-versions yet so we have to be creative. - name: Read .tool-versions uses: marocchino/tool-versions-action@v1 id: versions - name: Setup pnpm uses: pnpm/action-setup@v2 with: version: ${{ steps.versions.outputs.pnpm}}

Nice find, I love that. Let's do it in a follow-up PR!

oh nice, that's awesome!

.github/workflows/scip-typescript.yml

.tool-versions

WORKSPACE

dev/ci/integration/setup-deps.sh

dev/ci/pnpm-install-with-retry.sh

doc/dev/background-information/ci/development.md

eseliger · 2023-01-11T13:58:24Z

I was wondering if we can install a custom yarn shim somehow that only does echo "Use pnpm instead" && exit 1 or so but I wouldn't know where to put it so that it is in everyone's PATH and takes precedence over the asdf installation one 🤔

☝️ that's a great idea

// the bundlesize thing concerns me a bit, there's no free drinks here, is it? 😆

eseliger

did a code pass, will do a functional test in a bit. LGTM! Thanks for working through this :)

eseliger · 2023-01-11T13:51:00Z

package.json

-      "client/*",
-      "dev/release"
-    ]
-  },
  "devDependencies": {


how come there are quite a few changes in the listed dependencies here?

With node_modules hoisting disabled, pnpm makes it impossible to use packages not specified in the package.json file. This change revealed several dependencies we use but didn't add to our dependencies list explicitly.

eseliger · 2023-01-11T13:51:41Z

pnpm-lock.yaml

@@ -1,6 +1,7 @@
 lockfileVersion: 5.4


were you able to keep exactly the same pinned versions that we had in yarn.lock? Just curious, not necessarily a blocker but would make this switch much less regression-prone.

Yes, the pnpm import transforms the yarn.lock file into the pnpm-lock.yaml, preserving pinned versions.

eseliger · 2023-01-11T13:54:08Z

client/web/webpack.config.js

@@ -233,6 +233,11 @@ const config = {
  resolve: {
    extensions: ['.mjs', '.ts', '.tsx', '.js', '.json'],
    mainFields: ['es2015', 'module', 'browser', 'main'],
+    fallback: {


why was this not required before?

It wasn't required before the package hoisting was disabled. We can see that the main-dry-run build with hoisting=true is green. My guess is that without hoisting some internal modules stopped having access to the hoisted implementations of these packages, that's why we now need to have fallbacks for them.

eseliger · 2023-01-11T13:55:40Z

client/vscode/webpack.config.js

@@ -205,7 +196,7 @@ const webviewConfig = {
            loader: 'worker-loader',
            options: { inline: 'no-fallback' },
          },
-          'ts-loader',
+          getBabelLoader(),


what's the difference here?

Hmm, interesting, the whole time we transpire vscode client with tsc instead of babel with TS preset?

Yes, I tried to find the context behind this decision but didn't discover anything that prevents us from switching to babel. This was found when we bumped into TS errors upon building the VSCode extension, and it should not happen because we do type-checking as a separate CI step.

eseliger · 2023-01-11T15:17:53Z

tried it locally, nothing broke :)

jbedard · 2023-01-11T17:35:41Z

client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.story.tsx

@@ -1,18 +1,13 @@
 import { Meta, Story } from '@storybook/react'
 import ParentSize from '@visx/responsive/lib/components/ParentSizeModern'

-import webStyles from '@sourcegraph/web/src/SourcegraphWebApp.scss'
-import { BrandedStory } from '@sourcegraph/wildcard/src/stories'
+import { BrandedStory } from '../../../../stories'


I don't think the *.story.tsx files are part of the @sourcegraph/wildcard package? So should import by name like previously, instead of relatively within the package...

We should update the configuration somehow to enable that. Typescript complains about absolute imports like that inside of the package because it cannot locate type definitions for @sourcegraph/wildcard/src/stories. Let's discuss and update it in a follow-up PR.

jest.config.base.js

vovakulikov

Tested manually, all standard flows work as expected (at least subset that I use), if CI is happy I'm happy too. Big thanks to @valerybugakov this is blazingly amazing

.github/workflows/lighthouse-production.yml-disabled

client/branded/package.json

vovakulikov · 2023-01-11T15:25:08Z

client/shared/src/testing/integration/context.ts

@@ -158,7 +158,7 @@ export const createSharedIntegrationTestContext = async <

    // Fail the test in the case a request handler threw an error,
    // e.g. because a request had no mock defined.
-    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as CdpAdapter
+    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as unknown as CdpAdapter


Just out of curiosity is pnpm migration related to this and other TypeScript types fixes in this PR?

This is a pnpm migration-related change that started happening after disabling the package hoisting.

It's related to the fact that I installed @types/pollyjs__adapter to fix another TS issue before doing this change.

vovakulikov · 2023-01-11T18:53:55Z

client/vscode/webpack.config.js

@@ -205,7 +196,7 @@ const webviewConfig = {
            loader: 'worker-loader',
            options: { inline: 'no-fallback' },
          },
-          'ts-loader',
+          getBabelLoader(),


Hmm, interesting, the whole time we transpire vscode client with tsc instead of babel with TS preset?

enterprise/cmd/frontend/pre-build.sh

dev/ci/integration/setup-deps.sh

valerybugakov · 2023-01-11T01:19:03Z

client/vscode/webpack.config.js

-        use: [
-          {
-            loader: 'ts-loader',
-          },
-        ],
+        use: [getBabelLoader()],


The VSCode extensions changes were reviewed here:

https://github.com/sourcegraph/sourcegraph/pull/46194

valerybugakov · 2023-01-11T01:20:30Z

dev/sg/dependencies/shared.go

-				Name:  "yarn",
-				Check: checkYarnVersion,
+				Name:  "pnpm",
+				Check: checkPnpmVersion,


Tested locally by running: go run ./dev/sg setup -check

dev/ci/pnpm-install-with-retry.sh

.tool-versions

valerybugakov · 2023-01-11T12:01:50Z

.github/workflows/licenses-check.yml

+    - uses: pnpm/action-setup@v2
+      id: pnpm-install
+      with:
+        version: 7.24.2


Nice find, I love that. Let's do it in a follow-up PR!

valerybugakov · 2023-01-12T02:11:03Z

client/shared/src/testing/integration/context.ts

@@ -158,7 +158,7 @@ export const createSharedIntegrationTestContext = async <

    // Fail the test in the case a request handler threw an error,
    // e.g. because a request had no mock defined.
-    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as CdpAdapter
+    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as unknown as CdpAdapter


This is a pnpm migration-related change that started happening after disabling the package hoisting.

valerybugakov · 2023-01-12T02:16:36Z

client/shared/src/testing/integration/context.ts

@@ -158,7 +158,7 @@ export const createSharedIntegrationTestContext = async <

    // Fail the test in the case a request handler threw an error,
    // e.g. because a request had no mock defined.
-    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as CdpAdapter
+    const cdpAdapter = polly.adapters.get(CdpAdapter.id) as unknown as CdpAdapter


It's related to the fact that I installed @types/pollyjs__adapter to fix another TS issue before doing this change.

valerybugakov · 2023-01-12T02:22:36Z

client/web/webpack.config.js

@@ -233,6 +233,11 @@ const config = {
  resolve: {
    extensions: ['.mjs', '.ts', '.tsx', '.js', '.json'],
    mainFields: ['es2015', 'module', 'browser', 'main'],
+    fallback: {


It wasn't required before the package hoisting was disabled. We can see that the main-dry-run build with hoisting=true is green. My guess is that without hoisting some internal modules stopped having access to the hoisted implementations of these packages, that's why we now need to have fallbacks for them.

valerybugakov · 2023-01-12T02:48:33Z

client/wildcard/src/components/Charts/components/stacked-meter/StackedMeter.story.tsx

@@ -1,18 +1,13 @@
 import { Meta, Story } from '@storybook/react'
 import ParentSize from '@visx/responsive/lib/components/ParentSizeModern'

-import webStyles from '@sourcegraph/web/src/SourcegraphWebApp.scss'
-import { BrandedStory } from '@sourcegraph/wildcard/src/stories'
+import { BrandedStory } from '../../../../stories'


We should update the configuration somehow to enable that. Typescript complains about absolute imports like that inside of the package because it cannot locate type definitions for @sourcegraph/wildcard/src/stories. Let's discuss and update it in a follow-up PR.

jest.config.base.js

valerybugakov and others added 17 commits December 23, 2022 19:08

web: move global styles to wildcard

05cace5

web: fix incorrect import path

fd0b552

Merge branch 'main' into vb/move-global-styles-to-wildcard

ecd7a2b

web: move global styles to wildcard

c182a5e

web: test pnpm

04a46f7

chore: migrate yarn command to pnpm

fd4d746

Merge commit '2b0f6a5be8dda4dfc9686016bd67d60dc92a4816' of https://gi…

1a35d26

…thub.com/sourcegraph/sourcegraph into contractors/SG-45990

chore: revert some changes + fix some CI check failing

28d2cbf

[SG-45970] Add client packages to workspaces dependencies (#45981)

abdee47

feat: add client packages to workspaces dependencies

chore: revert some redundant changes

0809096

chore: resolve conflicts

0541f28

fix: build-ts

5c7960d

fix: pnpm-install-with-retry.sh

5fbbf04

fix: esbuild

62b57be

chore: update yarn usages in github action

8903355

web: use the realpath for build config paths to support symlinks in w…

e6ee064

…ebpack loaders

web: drop yarn related folders and configs

5f56591

valerybugakov added the bazel label Jan 5, 2023

valerybugakov self-assigned this Jan 5, 2023

cla-bot bot added the cla-signed label Jan 5, 2023

github-actions bot added the team/code-exploration Issues owned by the Code Exploration team label Jan 5, 2023

valerybugakov added 2 commits January 5, 2023 11:52

web: update ignore files

83af6fe

web: remove redundant pnpm usages

854ee1b

valerybugakov added the aspect-dev label Jan 5, 2023

sourcegraph-tracking-issue-bot mentioned this pull request Jan 5, 2023

Bazel Aspect-dev: building the frontend #44909

Closed

76 tasks

[SG-45990] Migrate all the remaining yarn commands to pnpm (#46008)

b01db04

Base automatically changed from contractors/SG-45990 to vb/pnpm-test-2 January 6, 2023 03:16

valerybugakov added 2 commits January 6, 2023 11:40

Merge branch 'vb/pnpm-test-2' into vb/pnpm-test-3

f459652

web: drop redundant dependencies

da1faeb

valerybugakov requested review from a team and jbedard January 11, 2023 01:15

valerybugakov commented Jan 11, 2023

View reviewed changes

Merge branch 'main' into vb/pnpm-test-3

63478f5

web: update pnpm

5463031

valerybugakov changed the title ~~web: pnpm migration updates~~ web: migrate from yarn to pnpm Jan 11, 2023

web: fix wildcard import

b72f3d2

valerybugakov mentioned this pull request Jan 11, 2023

build: add @types/minimatch dependency #46258

Closed

web: remove redundant import

a926ca1

valerybugakov mentioned this pull request Jan 11, 2023

ignore: pnpm test #46275

Closed

philipp-spiess approved these changes Jan 11, 2023

View reviewed changes

web: add --frozen-lockfile to pnpm install calls on CI

22f60cf

eseliger approved these changes Jan 11, 2023

View reviewed changes

jbedard reviewed Jan 11, 2023

View reviewed changes

jest.config.base.js Outdated Show resolved Hide resolved

vovakulikov approved these changes Jan 11, 2023

View reviewed changes

valerybugakov added 4 commits January 12, 2023 10:12

web: drop the --ignore-engines flag

0ef3809

web: remove redundant comment

6845ab7

web: update jest transformIgnorePatterns

ac7d314

Merge branch 'main' into vb/pnpm-test-3

a765b31

valerybugakov commented Jan 12, 2023

View reviewed changes

valerybugakov merged commit 304acdb into main Jan 12, 2023

valerybugakov deleted the vb/pnpm-test-3 branch January 12, 2023 03:50

This was referenced Jan 12, 2023

Migrate all the remaining yarn commands to pnpm #45990

Closed

Add client packages to workspaces dependencies #45970

Closed

		# We have to hoist eslint packages to use them in the `.eslintrc` config.
		public-hoist-pattern[]=eslint

web: migrate from yarn to pnpm #46143

web: migrate from yarn to pnpm #46143

Conversation

valerybugakov commented Jan 5, 2023 • edited Loading

Context

Breaking changes

Test plan

sg-e2e-regression-test-bob commented Jan 5, 2023 • edited Loading

Bundle size report 📦

Choose a reason for hiding this comment

sourcegraph-bot commented Jan 11, 2023 • edited Loading

sourcegraph-bot commented Jan 11, 2023 • edited Loading

valerybugakov commented Jan 11, 2023

philipp-spiess 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

eseliger commented Jan 11, 2023

eseliger 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

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

Choose a reason for hiding this comment

eseliger commented Jan 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vovakulikov 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

web: migrate from `yarn` to `pnpm` #46143

web: migrate from `yarn` to `pnpm` #46143

valerybugakov commented Jan 5, 2023 •

edited

Loading

sg-e2e-regression-test-bob commented Jan 5, 2023 •

edited

Loading

sourcegraph-bot commented Jan 11, 2023 •

edited

Loading

sourcegraph-bot commented Jan 11, 2023 •

edited

Loading