search

penumbra-zone / web

Apache License 2.0
12 stars 15 forks source link

fix: bech32m package exports #1292

Closed VanishMax closed 3 months ago

VanishMax commented 3 months ago

Fixes the way packages are consumed: bech32m, protobuf, client, getters

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 65957ccb35422f2cc7a6f60302a2ae1da8ec1e4f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 16 packages | Name | Type | | ------------------------------- | ----- | | @penumbra-zone/bech32m | Minor | | @penumbra-zone/eslint-config | Minor | | @penumbra-zone/protobuf | Minor | | @penumbra-zone/getters | Minor | | @penumbra-zone/client | Minor | | minifront | Patch | | @penumbra-zone/perspective | Patch | | @penumbra-zone/query | Patch | | @penumbra-zone/services | Patch | | @penumbra-zone/storage | Patch | | @penumbra-zone/types | Patch | | @penumbra-zone/ui | Patch | | @penumbra-zone/wasm | Patch | | node-status | Patch | | @penumbra-zone/services-context | Patch | | @penumbra-zone/crypto-web | Patch |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

VanishMax commented 3 months ago

Weirdly, the linting and testing works locally. Any idea what causes it to fail in the CI?

jessepinho commented 3 months ago

Hm, when I test it locally, pnpm lint and pnpm test both succeed.

The lines that CI is complaining about are the ones where we return a call to bech32mAssetId(). When I "Go to definition" on those calls, it takes me to packages/bech32m/dist/passet.d.ts, which has a TypeScript error:

image

Perhaps tsconfig needs to be updated to accommodate the new dist directory?

turbocrime commented 3 months ago

we should talk about what's broken in the exports - it does work in example repos, and some consumers who had problems were actually using the incompatible node or stacks that did not support our compile target.

the way you're configuring exports here will not export types.

VanishMax commented 3 months ago

@grod220 @turbocrime @jessepinho This PR is ready with 4 package fixes: bech32m, protobuf, client, getters. Tested the outputs in multiple ways, now it should be all correct.

Learned the difference between the exports field in package.json and publishConfig.exports. Apparently, pnpm workspace imports use the first field and therefore I configured it to point to src/*.ts files, while publishConfig.exports point to dist/*.js and dist/*.d.ts files.

FYI, the lints and tests were failing because CI doesn't do pnpm build before running checks. Changing the exports field fixed the issues

jessepinho commented 3 months ago

@VanishMax I'd love to hear more about what you've learned during one of our weekly syncs. (And @turbocrime you too, if you're interested.) I feel like these issues have been plaguing us for a while and I don't really understand what goes into fixing them.