-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
feat: separate lodestar utils to node and browser #7039
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7039 +/- ##
============================================
- Coverage 49.25% 49.12% -0.13%
============================================
Files 578 577 -1
Lines 37443 37333 -110
Branches 2172 2159 -13
============================================
- Hits 18441 18339 -102
+ Misses 18962 18954 -8
Partials 40 40 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
}, | ||
"./node": { | ||
"import": "./lib/node.js" | ||
} |
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 main change of this PR
need to work on a solution to try NodeJS version first, if not fallback to browser version |
replaced by #7060 |
Motivation
@lodestar/utils
we want to have specific apis for browser. In the future we can also enhance to have switchable implementation Use Buffer.from for NodeJS fromHexString ssz#283toHex()
andtoHexString()
which is confusing in terms of environment unless we look into the implementationtoRootHex()
uses Buffer which does not work in browser, see also fix: allocate shared buffer at runtime to fix browser compatibility #7023toRootHex()
for browser but get naming conflectDescription
@lodestar/utils
to@lodestar/utils/node
for node and@lodestar/utils/browser
for browser@lodestar/utils
for common apisa prerequisite for #7036
cc @nflaig