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

apply snarkjs performance improvement to semaphore package #1787

Closed
wants to merge 8 commits into from

Conversation

ichub
Copy link
Contributor

@ichub ichub commented Jun 13, 2024

No description provided.

@ichub ichub force-pushed the ivan/better-semaphore branch from 0d5056b to e7d6776 Compare June 13, 2024 23:19
@robknight
Copy link
Member

I wonder if this is going to get confusing if the underlying package gets upgraded in the future, and whether we couldn't maintain patches using something like this: https://www.npmjs.com/package/patch-package

@ichub
Copy link
Contributor Author

ichub commented Jun 13, 2024

It probably will get confusing, at which point I'd be down to check out patch-package as an alternative. I think for now this is fine. My goal is to reduce friction for the ongoing event ASAP.

@artwyman
Copy link
Member

I wonder if this is going to get confusing if the underlying package gets upgraded in the future, and whether we couldn't maintain patches using something like this: https://www.npmjs.com/package/patch-package

I really hope that all of this patching is a short-term necessity which will go away via upstreaming a fix to snarkjs. Otherwise yes, I think managing these forks every time a dependency updates is going to be come unmanageable.

[patch-package](https://www.npmjs.com/package/patch-package) sounds cool to check out if we can't upstream our fix for some reason.

@ichub
Copy link
Contributor Author

ichub commented Jun 14, 2024

https://linear.app/0xparc-pcd/issue/0XP-954/follow-up-on-snarkjs-fixes-open-sourcely

@ichub
Copy link
Contributor Author

ichub commented Jun 18, 2024

superceded by #1797, thanks @robknight for the patch-package suggestion. it turned out to be untenable for me to patch semaphore the same way i was doing snarkjs (urgh) so I used patch-package which ended up being a lot simpler!

@ichub ichub closed this Jun 18, 2024
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.

3 participants