-
Notifications
You must be signed in to change notification settings - Fork 163
bump @cosmjs/* packages to latest version #579
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
base: main
Are you sure you want to change the base?
Conversation
|
thanks for the PR! Looks like one dependency is missing? |
|
Heyhey. You found a bug in CosmJS there. The min version of @noble/hashes we need is 1.8.0. See cosmos/cosmjs#1884 You can fix this by manually deleting the block and then run |
|
@webmaster128 Thank you for commenting here! I think the fix you suggest won't work when users install cosmos-kit in their apps, right? I've been looking at your recent changes and I see that v0.37-rc0 contains the fix for |
|
@pyramation I've been trying to fix it locally but it seems that this is not fixable without a new cosmjs release. There are also dependencies like For example: and and |
When a user newly installs cosmos-kit they always install the latest version 1.8.0. Then that case they have no problem. The only issue is old existing lockfiles where earlier versions are locked. That being said, I just released 0.36.2 which contains the fix from the PR I linked above. |
@noble/hashes ^1.8.0 supports imports with and without extension. So that's not an issue anymore once you have ^1.8.0 as the required version range. |
|
Thank you @webmaster128 for assistance and responsiveness! |
3e7534f to
873e8c8
Compare
873e8c8 to
d02fd83
Compare
|
@pyramation I updated gha to run all available tests in repo, also fixed some issues in other packages. Could you please take a look and run gha again? |
| // Polyfill for TextEncoder/TextDecoder required by @cosmjs packages | ||
| const { TextEncoder, TextDecoder } = require('util'); | ||
| global.TextEncoder = TextEncoder; | ||
| global.TextDecoder = TextDecoder; |
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.
Why do you think this addition is needed? TextEncoder is widely adopted for a long time. Would be really strange if your JS envirnment does not have it.
https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder
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, I know. I use [email protected] but probably the issue is in jest env. Tests failed with the error that this stuff is not available
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’ll double check a bit later whether the issue is in env. Because jsdom doesn’t even provide fetch api which is also widely available.
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.
Okay I am not at all familiar with those. But I think we should adapt the comment a bit to highlight where the limitation is (and when we can remove the polyfill).
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.
related issue in jsdom-> jsdom/jsdom#2524
Why
comsjs < 0.34 has security vulnerability introduced by elliptic package. cosmos/cosmjs#1272. Fixes #576