Skip to content
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

chore(js): add expo example app and move to pnpm #247

Merged

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented May 8, 2024

Rust side changes

  • Updated the secure-env library as there were some issues with Android in a non cargo-ndk managed app.
  • Added a HasKeyBackend trait which defaults to KeyBackend::Software. This is done so we can correctly create P256 Software vs Hardware keys.

@berendsliedrecht
Copy link
Contributor Author

@swcurran snyk seems to fail, but I cannot access the project page on their website. Could you help with this? I just need to know which dependencies are the issue.

@swcurran
Copy link
Contributor

swcurran commented May 10, 2024

@berendsliedrecht — is this helpfuil? Not sure it is what you want. It what I can see, and the only related link is the settings gear, but that doesn’t seem to be helpful.

image

@berendsliedrecht
Copy link
Contributor Author

@berendsliedrecht — is this helpfuil? Not sure it is what you want. It what I can see, and the only related link is the settings gear, but that doesn’t seem to be helpful.

image

Hmm it wants it to be built with yarn. Do we have some setting to set the package manager? Pnpm-lock.yaml is equal to yarn.lock

@swcurran
Copy link
Contributor

I don’t know about any of this stuff, so stumbling around. When I hit the “gear” from the image above, and then in the top right “Open in GitHub” — it takes me to this page: https://github.com/hyperledger/aries-askar/blob/main/wrappers/javascript/package.json where there are a number of references to yarn. Is that file correct?

@TimoGlastra
Copy link
Contributor

Requested us to be added @berendsliedrecht: hyperledger/governance#259

@berendsliedrecht berendsliedrecht marked this pull request as ready for review May 14, 2024 10:59
TimoGlastra
TimoGlastra previously approved these changes May 14, 2024
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice 👍

@@ -0,0 +1,71 @@
root: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be nice to also move this to biome so we can drop prettier + eslint

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, wanted to do that at a later stage.

"repository": {
"type": "git",
"url": "https://github.com/hyperledger/aries-askar",
"url": "https://github.com/hyperledger/aries-askar-rs",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"url": "https://github.com/hyperledger/aries-askar-rs",
"url": "https://github.com/hyperledger/aries-askar",

Comment on lines +41 to +64
? number
: Type[Property] extends Record<string, unknown> | unknown[]
? string
: Type[Property] extends Buffer | Uint8Array | Key | ArcHandle | Jwk
? Buffer
: Type[Property] extends Callback
? Callback
: Type[Property] extends CallbackWithResponse
? CallbackWithResponse
: Type[Property] extends boolean | undefined
? number
: Type[Property] extends unknown[] | undefined
? string
: Type[Property] extends Record<string, unknown> | undefined
? string
: Type[Property] extends Date | undefined
? number
: Type[Property] extends string | undefined
? string
: Type[Property] extends number | undefined
? number
: Type[Property] extends Uint8Array | undefined
? Buffer
: unknown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it 😆

@berendsliedrecht
Copy link
Contributor Author

berendsliedrecht commented May 15, 2024

Going a bit through the Snyk issue again, it seems that they do not have support for PNPM (snyk/nodejs-lockfile-parser#111). Can we disable it for the JS projects and just have dependabot deal with vunelerabilities?

@swcurran

@swcurran
Copy link
Contributor

@WadeBarnes — can you help here? You have a way better handle on this than I. Thanks!

@swcurran swcurran requested a review from WadeBarnes May 15, 2024 14:42
@swcurran
Copy link
Contributor

@WadeBarnes — I mean about the Snyk comment — #247 (comment)

@WadeBarnes
Copy link
Contributor

It seems Snyk is adding support for pnpm; snyk/nodejs-lockfile-parser#111 (comment)

Some additional questions to establish some background (mostly for me since the setup is not obvious):

  • How does Snyk come into play with what's being discussed here?
  • How is Snyk integrated with the project?
    • At what level?
    • Directly or indirectly?

@berendsliedrecht berendsliedrecht force-pushed the pnpm-and-example-app branch 4 times, most recently from a8cf96c to b4851df Compare May 22, 2024 10:46
@berendsliedrecht berendsliedrecht marked this pull request as draft May 22, 2024 10:53
@berendsliedrecht berendsliedrecht force-pushed the pnpm-and-example-app branch 2 times, most recently from f3bd36e to dbd5a9b Compare May 22, 2024 13:53
@berendsliedrecht berendsliedrecht marked this pull request as ready for review June 4, 2024 11:19
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
@berendsliedrecht berendsliedrecht force-pushed the pnpm-and-example-app branch 2 times, most recently from cd6d0c9 to 849cd67 Compare June 4, 2024 11:24
TimoGlastra
TimoGlastra previously approved these changes Jun 4, 2024
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except for pushing android/ios folders for example app

@@ -0,0 +1,6 @@
<resources>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the example app an Expo app and not push the prebulid outputs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ofcourse! I think the gitignore was picked up later while rebasing. Shouldn't have pushed it!

@berendsliedrecht
Copy link
Contributor Author

@andrewwhitehead could you review the rust side? Quite minimal changes, mainly a new trait HasKeyBackend which every key implements with a default of KeyBackend::Software.

@TimoGlastra
Copy link
Contributor

DCO seems to be messed up. Maybe you need to squash and force push?

berendsliedrecht and others added 4 commits June 4, 2024 15:05
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
@berendsliedrecht
Copy link
Contributor Author

@WadeBarnes

Some additional questions to establish some background (mostly for me since the setup is not obvious):

How does Snyk come into play with what's being discussed here?
How is Snyk integrated with the project?
At what level?
Directly or indirectly?

Sadly I am not a Snyk expert so I cannot answer these questions. I think it is integrated indirectly just by being under the Hyperledger organization (not a 100% sure, but I cannot seem to find any Snyk setup in this repo). IMO we can just disable and let dependabot deal with the specific issues for JS. This is blocking quite some work now, also for Credo.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Jun 7, 2024

@ryjones
Copy link
Contributor

ryjones commented Jun 7, 2024

wrappers/javascript/package.json

Missing required yarn.lock.

I've removed the integration

@TimoGlastra
Copy link
Contributor

@berendsliedrecht Made a similar PR in indy vdr repo, but also updated to Biome and vitest there (lest config/setup). Maybe we can do the same here as well.

@TimoGlastra TimoGlastra merged commit 9ff1271 into openwallet-foundation:main Jun 8, 2024
29 of 30 checks passed
jamshale pushed a commit to jamshale/aries-askar that referenced this pull request Aug 18, 2024
…on#247)

Signed-off-by: Berend Sliedrecht <sliedrecht@berend.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants