-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix client package #1027
fix client package #1027
Conversation
55a240c
to
90a2364
Compare
working demo with this client package here https://github.com/turbocrime/nextjs-penumbra-client-example |
@@ -5,7 +5,7 @@ export enum PenumbraRequestFailure { | |||
|
|||
export const PenumbraSymbol = Symbol.for('penumbra'); | |||
|
|||
export interface PenumbraProvider { | |||
export interface PenumbraInjection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the thinking behind the rename? Is this standard with other wallets? I feel like the Provider
naming is more common in things I've encountered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provider
naming is common for react components that provide something to child components, which felt awkward while writing the demo and using other Provider
-named things, so i renamed this.
"prebuild": "$npm_execpath run clean", | ||
"prepack": "$npm_execpath run build" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's $npm_execpath
do here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$npm_execpath
will refer to npm/pnpm/yarn/whatever was used to call the script. it seems like this is the only cross-package-manager way for one package script to call another. these scripts probably don't matter because a consumer won't call them outside of our monorepo, but in general when writing a package for publication it's better to not name an explicit npm/pnpm/yarn in scripts
i'm going to add a changeset patch bump to bech32m to calm down the peer dependency, and then merge this |
5070eaa
to
d6d65f1
Compare
some more client package work in another PR but this is enough for the demo