-
Notifications
You must be signed in to change notification settings - Fork 23
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
Improve DX when working with WASM dependencies #299
Improve DX when working with WASM dependencies #299
Conversation
9a44cd9
to
8be6ed4
Compare
Codecov Report
@@ Coverage Diff @@
## monorepo #299 +/- ##
============================================
+ Coverage 65.23% 65.67% +0.43%
============================================
Files 48 48
Lines 5218 5223 +5
Branches 35 28 -7
============================================
+ Hits 3404 3430 +26
+ Misses 1808 1787 -21
Partials 6 6
|
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.
LGTM overall, just a few thoughts
examples/nextjs/README.md
Outdated
This is a [Next.js](https://nextjs.org/) project bootstrapped with | ||
[`create-next-app`](https://github.com/vercel/next.js/tree/canary/packages/create-next-app). | ||
|
||
## Getting Started |
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.
Before running the dev server, it is necessary to install the packages, right? I tried with npm install, but I got an error:
% npm i
npm ERR! code EUNSUPPORTEDPROTOCOL
npm ERR! Unsupported URL Type "workspace:": workspace:*
But it worked with pnpm: pnpm install
Should we add pnpm install
to this section?
examples/nextjs/README.md
Outdated
|
||
Open [http://localhost:3000](http://localhost:3000) with your browser to see the | ||
result. | ||
|
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.
Should we add a note about Metamask (if it is being used) should have Mumbai as the network selected ?
51f2439
to
15ea563
Compare
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.
I do think await initialize()
is an improvement, although I do wish that the error raised when executing nucypher-core functions without calling it was more informative. Is it possible to catch and re-raise? In any case good documentation will mitigate appropriately in my opinion. Appoved!
🤠
const [bob, setBob] = useState<Bob | undefined>(); | ||
const [policy, setPolicy] = useState<EnactedPolicy>(); | ||
|
||
const initNucypher = async () => { |
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.
This is an internal function, right? Name is not very descriptive, but if it's only a local thing, it's fine.
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.
Yes, it's only used in this example as a helper method
@@ -4,6 +4,7 @@ | |||
"license": "GPL-3.0-only", | |||
"author": "Piotr Roslaniec <p.roslaniec@gmail.com>", | |||
"scripts": { | |||
"postinstall": "pnpm build || true && echo 'Ignoring build errors on setup until we publish the first version'", |
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.
We should track this
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.
Opened #307
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.
Tracked here: #308
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.
Beat me to it
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.
😆
Type of PR:
Required reviews:
What this does:
@nucypher/nucypher-core
NPM package nucypher-core#83nucypher-ts
user will have to call theawait initialize()
method imported fromnucypher-ts
Issues fixed/closed:
@nucypher/nucypher-core
NPM package nucypher-core#83Why it's needed:
nucypher-ts
usersNotes for reviewers:
nucypher-ts
version because of how the integration works now.@nucypher/nucypher-core
packageawait initialize()
an improvement from having to configure bundler? You can learn more about the trade-offs in the related PR: Inline WASM in@nucypher/nucypher-core
NPM package nucypher-core#83./examples
to compare and contrast this change with the previous DX