-
Notifications
You must be signed in to change notification settings - Fork 759
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
Clean up dev dependencies #3710
Conversation
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.
Changes LGTM, however the examples also seem to have a dependency on ethersproject/abi
Also, the browser tests fail (re-ran it once, gave same error):
Run npm run install-browser-deps
> install-browser-deps
> npm install webdriverio @vitest/browser
npm error Cannot read properties of null (reading 'edgesOut')
npm error A complete log of this run can be found in: /home/runner/.npm/_logs/2024-09-29T21_49_25_024Z-debug-0.log
Error: Process completed with exit code 1.
See: https://github.com/ethereumjs/ethereumjs-monorepo/actions/runs/11088816287/job/30825710880?pr=3710
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.
Two nits/questions
@@ -68,7 +67,7 @@ | |||
"@rollup/rollup-linux-x64-gnu": "*" | |||
}, | |||
"peerDependencies": { | |||
"@vitest/browser": "2.1.0", | |||
"@vitest/browser": "^2.1.0", | |||
"webdriverio": "^8.39.0" |
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.
If the install-browser-deps
now installs specific versions of these dependencies, should we then also remove the caret here to ensure this specific version is installed?
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 tried this but when I run the npm run install-browser-deps
script, it automatically adds the caret back (no idea why). It should be okay though since we're installing specific versions from the script.
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.
ACK
Cool, great! 🙂 🙏 That's really a nice step towards better assured browser compatibility, so that one not always have to doubt if/why these plugins/polyfills are needed. |
Removes
vitest-plugin-node-polyfills
from our dev dependencies and fixes related testsRemoves
@ethersproject/abi
since it was only used to derive calldata for a single test