-
Notifications
You must be signed in to change notification settings - Fork 835
Use noble
bytes conversion utilities internally
#3698
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
Conversation
Browser tests still failing |
Update: just read up on Discord, let me know if I should still deactivate the browser tests. |
) | ||
const status = p.decodeStatus({ | ||
chainId: [0x01], | ||
chainId: Uint8Array.from([0x01]), |
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 is this change now necessary?
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.
the function accepted arrays before. Now it is limited to u8a
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.
Furthermore, this property is supposed to be a Uint8Array if you look at the types.
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
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
], | ||
optimizeDeps: { | ||
exclude: ['kzg-wasm'], | ||
include: ['vite-plugin-node-polyfills/shims/buffer', 'vite-plugin-node-polyfills/shims/global', 'vite-plugin-node-polyfills/shims/process'] |
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.
At first look, these additions seem to go against our as-pure-as-possible-browser-support direction strategy. 🤔
Is this something to further investigate (why needed, what changed, where to adjust)?
(I just created a non-minifed VM (so not: EVM) bundle as a first test and searched for "Buffer" but did not find anything suspicious)
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 would agree that this was my surface thought as well. We have these various shims by default as a part of the vite-plugin-node-polyfills
dep for running our tests and vitest was having trouble reloading them or something (saw these messages about optimizing these deps over and over again in the logs) and the suggested fix was to add them to the optimizeDeps
list in our config. Given the effort on getting rid of polyfills, we might be able to just get rid of that plugin though I'm not 100% sure. Worth researching.
Addresses #3693 since the noble hex to bytes conversion utility is about 2x ours.