Add changeset support (npm publishing part 1) #672

grod220 · 2024-03-05T16:29:56Z

Closes #138

We have a few consumers interested in making requests to the extension. That means we need to start publishing our packages to npm 📦. Followed the recommendations from turborepo docs in using changesets for versioning+publishing.

Marked packages we don't anticipate publishing with private
Uses changeset github action
Added deployment docs
Adds compilation for a few packages so they can serve as independent packages

Specifically targeting client package to publish first. However, need to handle types, getters, constants, and transport-dom as well due to dep tree.

turbocrime · 2024-03-06T00:12:07Z

it seems unnecessary to bother with CJS support. both node and the browser have full ESM support now

grod220 · 2024-03-12T17:50:49Z

.eslintrc.js

@@ -1,7 +1,7 @@
 module.exports = {
  root: true,
  // This tells ESLint to load the config from the package `eslint-config-custom`
-  extends: ['custom'],
+  extends: ['@penumbra-zone/eslint-config-custom'],


Note: packages published externally need to have their dependencies published as well

there's a couple options here which don't require publishing an eslint config package

transition to eslint flat config, so it is no longer a package-level thing

programmatically remove the eslint devDeps with a prepack configuration

apps/extension/src/approve-origin.ts

grod220 · 2024-03-12T17:57:17Z

packages/constants/src/chains.ts

+export interface Chain {
+  displayName: string;
+  chainId: string;
+  ibcChannel: string;
+  iconUrl: string;
+}


We had a hidden cyclical dependency

grod220 · 2024-03-12T17:58:47Z

packages/crypto/package.json

    "lint": "eslint \"**/*.ts*\"",
    "test": "vitest run"
  },
  "dependencies": {
    "bip39": "^3.1.0",
-    "crypto-js": "^4.2.0"
+    "crypto-js": "^4.2.0",
+    "@penumbra-zone/types": "workspace:*"


Two reasons why @penumbra-zone/types was put back in the local directories:

Cyclical dependencies (not detected when in parent package.json)

Turbo's automatic detection of build dependencies did not work without it

if this is correct we should probably raise an issue with turbo

grod220 · 2024-03-12T18:00:44Z

packages/router/src/grpc/view-protocol-server/fmd-parameters.test.ts

+      getWalletServices: vi.fn(() =>
+        Promise.resolve({ indexedDb: mockIndexedDb }),
+      ) as MockServices['getWalletServices'],


Given we have to build all these packages, type errors are now getting caught that went unchecked before

grod220 · 2024-03-12T18:01:21Z

packages/router/src/grpc/view-protocol-server/ephemeral-address.ts

 import { servicesCtx } from '../../ctx';

 import { getEphemeralByIndex } from '@penumbra-zone/wasm';

-export const ephemeralAddress: Impl['ephemeralAddress'] = async (req, ctx) => {
+export const ephemeralAddress: ViewImpl['ephemeralAddress'] = async (req, ctx) => {


Easier to re-export in barrel file if they aren't all named the same Impl var

the type is for local use, so we should probably break this out into another file that isn't exported from the package

also, now that we're no longer doing barrel this change could revert

grod220 · 2024-03-12T18:02:23Z

packages/services/package.json

@@ -1,16 +1,27 @@
 {
  "name": "@penumbra-zone/services",
-  "version": "1.0.0",
+  "version": "1.0.1",


These packages might have strange version numbers, but that's just because I've been publishing to test it out. Don't think we need to start on a clean 1.0.0 anyway.

grod220 · 2024-03-12T18:35:57Z

packages/client/package.json

+    ".": "./dist/index.js"
+  },
+  "scripts": {
+    "build": "pnpm clean && tsc && vite build",


Conundrum, vite build is not idempotent for some reason. Exclude in tsconfig is not respected and therefore tries to re-bundle dist.

{ "extends": "@penumbra-zone/tsconfig/base.json", "exclude": ["node_modules", "dist"], "compilerOptions": { "outDir": "dist" } }

calling vite build manually demonstrates that it is in fact idempotent. it's actually a tsc problem - the typescript compiler is refusing to clobber existing output

after some experimenting, it seems that tsc is unnecessary, and vite build by itself doesn't complain. if we're using vite-plugin-dts we should either not call tsc or use noEmit in tsconfig anyway

I think we should keep tsc. It has managed to find type errors in a few packages that are otherwise going unnoticed.

ok - then let's use tsconfig noEmit

grod220 · 2024-03-12T18:45:07Z

.github/workflows/turbo-ci.yml

@@ -30,7 +30,7 @@ jobs:

  turbo-lint:
    name: Lint
-    runs-on: buildjet-2vcpu-ubuntu-2204
+    runs-on: buildjet-4vcpu-ubuntu-2204


We need more ram now...

turbocrime

it's wild that turbo doesn't handle top-level workspace dependencies, but maybe that's just a sign that our types package is overgrown.

i have several suggestions and i'll either PR some changes here or make a separate example branch.

turbocrime · 2024-03-12T20:06:26Z

.eslintrc.js

@@ -1,7 +1,7 @@
 module.exports = {
  root: true,
  // This tells ESLint to load the config from the package `eslint-config-custom`
-  extends: ['custom'],
+  extends: ['@penumbra-zone/eslint-config-custom'],


there's a couple options here which don't require publishing an eslint config package

transition to eslint flat config, so it is no longer a package-level thing

programmatically remove the eslint devDeps with a prepack configuration

apps/extension/src/approve-origin.ts

apps/extension/src/clients.ts

packages/crypto/index.ts

turbocrime · 2024-03-12T20:36:57Z

packages/crypto/package.json

    "lint": "eslint \"**/*.ts*\"",
    "test": "vitest run"
  },
  "dependencies": {
    "bip39": "^3.1.0",
-    "crypto-js": "^4.2.0"
+    "crypto-js": "^4.2.0",
+    "@penumbra-zone/types": "workspace:*"


if this is correct we should probably raise an issue with turbo

apps/extension/tsconfig.json

turbocrime · 2024-03-12T21:13:20Z

packages/client/package.json

+    ".": "./dist/index.js"
+  },
+  "scripts": {
+    "build": "pnpm clean && tsc && vite build",


calling vite build manually demonstrates that it is in fact idempotent. it's actually a tsc problem - the typescript compiler is refusing to clobber existing output

after some experimenting, it seems that tsc is unnecessary, and vite build by itself doesn't complain. if we're using vite-plugin-dts we should either not call tsc or use noEmit in tsconfig anyway

apps/extension/src/content-scripts/injected-penumbra-global.ts

grod220 · 2024-03-13T11:27:14Z

A new starting point in attempt to publish client package. Required:

client
constants
getters
polyfills
transport-dom
types

Reverted

crypto
estlint-custom-config
query
router
services
storage
tailwind-config
transport-chrome
ts-config

Surprised that eslint/ts-config is no longer giving publishing issues. I suppose it's because they are parent package.json deps now? Can't remember the issue before.

grod220 · 2024-03-13T14:47:44Z

Removed the barrel files for these packages we are now exposing. Thought, it kinda feels... wrong. I feel like there is major issue in javascript land that a clean API and performant imports are not compatible. Regardless, this is something perhaps we can explore ongoingly.

grod220 · 2024-03-13T14:53:34Z

packages/client/package.json

+    ".": "./dist/prax.js",
+    "./global": "./dist/global.js",
+    "./src/global": "./dist/global.js",
+    "./get-port": "./dist/get-port.js"


So this is kinda the demo of how we can make external packages a bit nicer (given the circumstances).

"./src/global": "./dist/global.js", <-- the way that internal packages require the exports
"./global": "./dist/global.js", <-- external packages get the ability to lose the /src/ part of the import
".": "./dist/prax.js", <-- the default . is the only harmonized internal/external import that drops the need for /src/. In this case, gave that honor to prax.js as it's likely the most common import for this package.

Note: I did not do this with any other package. The redundancy with say @penumbra-zone/types would be massive and difficult to maintain. Likely for later we need to consider a specific external/internal package.json.

from my example

web/packages/client/package.json

Lines 10 to 28 in b1a2a33

"exports": {

".": "./src/index.ts",

"./*": "./src/*.ts"

},

"publishConfig": {

"files": [

"dist"

],

"exports": {

".": {

"import": "./dist/src/index.js",

"types": "./dist/src/index.d.ts"

},

"./*": {

"import": "./dist/src/*.js",

"types": "./dist/src/*.d.ts"

}

}

},

the types package, which is private in example and needs no publication, remains unchanged

The types package is a dependency of this package. If we do not publish it, it will throw an error when the external app goes to npm install it

turbocrime

this still seems overcomplicated with many unnecessary changes, and we really don't need to be publishing internal packages like tsconfig. i'll make a PR cleaning up tsconfig today

turbocrime · 2024-03-13T15:31:45Z

packages/client/package.json

+    ".": "./dist/prax.js",
+    "./global": "./dist/global.js",
+    "./src/global": "./dist/global.js",
+    "./get-port": "./dist/get-port.js"


from my example

web/packages/client/package.json

Lines 10 to 28 in b1a2a33

"exports": {

".": "./src/index.ts",

"./*": "./src/*.ts"

},

"publishConfig": {

"files": [

"dist"

],

"exports": {

".": {

"import": "./dist/src/index.js",

"types": "./dist/src/index.d.ts"

},

"./*": {

"import": "./dist/src/*.js",

"types": "./dist/src/*.d.ts"

}

}

},

the types package, which is private in example and needs no publication, remains unchanged

turbocrime · 2024-03-13T15:32:41Z

packages/client/package.json

+    ".": "./dist/index.js"
+  },
+  "scripts": {
+    "build": "pnpm clean && tsc && vite build",


ok - then let's use tsconfig noEmit

turbocrime · 2024-03-13T15:34:58Z

packages/router/src/grpc/view-protocol-server/ephemeral-address.ts

 import { servicesCtx } from '../../ctx';

 import { getEphemeralByIndex } from '@penumbra-zone/wasm';

-export const ephemeralAddress: Impl['ephemeralAddress'] = async (req, ctx) => {
+export const ephemeralAddress: ViewImpl['ephemeralAddress'] = async (req, ctx) => {


the type is for local use, so we should probably break this out into another file that isn't exported from the package

also, now that we're no longer doing barrel this change could revert

turbocrime · 2024-03-13T15:36:38Z

packages/router/src/grpc/view-protocol-server/fmd-parameters.test.ts

+      getWalletServices: vi.fn(() =>
+        Promise.resolve({ indexedDb: mockIndexedDb }),
+      ) as MockServices['getWalletServices'],


turbocrime · 2024-03-13T15:37:58Z

packages/types/src/translators/address-view.test.ts

-        case: 'visible',
+        case: 'decoded',


what is this change

A type error that's been fixed

turbocrime · 2024-03-13T15:38:41Z

package.json

-    "all-check": "pnpm install && pnpm compile && pnpm format-check && pnpm lint && pnpm test && pnpm download-keys && pnpm build && pnpm format-check:rust && pnpm lint:rust && pnpm test:rust"
+    "changeset": "changeset",
+    "changeset:publish": "changeset publish",
+    "all-check": "pnpm install && pnpm clean && pnpm compile && pnpm format-check && pnpm lint && pnpm test && pnpm download-keys && pnpm build && pnpm format-check:rust && pnpm lint:rust"


i think having clean in all-check makes dev difficult because it blows away keys and results in a lot of redownload

apps/extension/src/message/popup.ts

apps/extension/src/popup.ts

turbocrime · 2024-03-13T15:44:39Z

apps/minifront/src/state/staking/index.test.ts

@@ -1,5 +1,5 @@
 import { beforeEach, describe, expect, it, vi } from 'vitest';
-import { StoreApi, UseBoundStore, create } from 'zustand';
+import { create, StoreApi, UseBoundStore } from 'zustand';


it seems like a good chunk of these changes are due to import sorting, can you turn that off

lol definitely not

grod220 · 2024-03-13T16:25:37Z

this still seems overcomplicated with many unnecessary changes

Most of these changes were due to the fact we are now building and it exposed a number of typing issues.

and we really don't need to be publishing internal packages like tsconfig

It is not, it's a private package

turbocrime · 2024-03-13T16:35:29Z

the polyfill publish is broken

turbocrime

merge it create issues for further concerns

turbocrime force-pushed the changset-npm branch from 0c98ca6 to ae57b7d Compare March 5, 2024 23:44

turbocrime mentioned this pull request Mar 6, 2024

npm packages #685

Closed

grod220 force-pushed the changset-npm branch 7 times, most recently from b345281 to f18494a Compare March 12, 2024 16:53

grod220 commented Mar 12, 2024

View reviewed changes

apps/extension/src/approve-origin.ts Show resolved Hide resolved

grod220 commented Mar 12, 2024

View reviewed changes

grod220 requested a review from turbocrime March 12, 2024 18:13

grod220 commented Mar 12, 2024

View reviewed changes

turbocrime suggested changes Mar 12, 2024

View reviewed changes

grod220 mentioned this pull request Mar 13, 2024

Tracking issue: NPM publishing #729

Closed

20 tasks

grod220 self-assigned this Mar 13, 2024

grod220 added the ci/cd Deployment and github action related label Mar 13, 2024

grod220 changed the title ~~Add changeset support (npm publishing)~~ Add changeset support (npm publishing part 1) Mar 13, 2024

grod220 force-pushed the changset-npm branch 3 times, most recently from 58c963f to 917fce8 Compare March 13, 2024 11:20

grod220 force-pushed the changset-npm branch from 917fce8 to feca6c1 Compare March 13, 2024 11:27

grod220 commented Mar 13, 2024

View reviewed changes

grod220 mentioned this pull request Mar 13, 2024

Improve external deps import paths #736

Closed

grod220 requested a review from turbocrime March 13, 2024 15:22

turbocrime suggested changes Mar 13, 2024

View reviewed changes

grod220 force-pushed the changset-npm branch from a1144fe to 60f0815 Compare March 13, 2024 16:35

grod220 force-pushed the changset-npm branch from 306402c to 68f3307 Compare March 13, 2024 17:02

turbocrime approved these changes Mar 13, 2024

View reviewed changes

grod220 added 2 commits March 13, 2024 21:47

Add changeset support (part 1)

929e4f5

no emit

dfcdabf

grod220 force-pushed the changset-npm branch from 94f761f to dfcdabf Compare March 13, 2024 20:51

grod220 merged commit 4ef2ef7 into main Mar 13, 2024
6 checks passed

grod220 deleted the changset-npm branch March 13, 2024 21:02

This was referenced Mar 13, 2024

Wildcard exports #750

Closed

Dropping the /src/ #762

Closed

	"exports": {
	".": "./src/index.ts",
	"./": "./src/.ts"
	},
	"publishConfig": {
	"files": [
	"dist"
	],
	"exports": {
	".": {
	"import": "./dist/src/index.js",
	"types": "./dist/src/index.d.ts"
	},
	"./*": {
	"import": "./dist/src/*.js",
	"types": "./dist/src/*.d.ts"
	}
	}
	},

Add changeset support (npm publishing part 1) #672

Add changeset support (npm publishing part 1) #672

Conversation

grod220 commented Mar 5, 2024 • edited Loading

turbocrime commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 Mar 12, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grod220 Mar 12, 2024 • 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

turbocrime 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

grod220 commented Mar 13, 2024 • edited Loading

grod220 commented Mar 13, 2024

grod220 Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turbocrime 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

grod220 commented Mar 13, 2024

turbocrime commented Mar 13, 2024

turbocrime left a comment

Choose a reason for hiding this comment

grod220 commented Mar 5, 2024 •

edited

Loading

grod220 Mar 12, 2024 •

edited

Loading

grod220 Mar 12, 2024 •

edited

Loading

grod220 commented Mar 13, 2024 •

edited

Loading

grod220 Mar 13, 2024 •

edited

Loading