feat: new IPLD Format API #52
Conversation
ghost
assigned
vmx
force-pushed
the
ipld-format-cleanup-other
branch
from
af798c8 to
12ca4bb
Compare
|
@rvagg This PR is based on the work at #51 but it's now using the approach you proposed. I'm not just creating new objects and store the original object in a property called I also improved the tests to check for the types of the fields when they are resolved. It would be great if you could prioritise the review of this PR. |
Codecov Report
@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 96.84% 99.36% +2.52%
==========================================
Files 22 19 -3
Lines 792 791 -1
==========================================
+ Hits 767 786 +19
+ Misses 25 5 -20
Continue to review full report at Codecov.
|
|
tangential: why are we persisting with the term "ommer" when eth seems to be preferring "uncle" now and even the ethereumjs library looks like it's using "uncle" but we're turning it back in to "ommer". I don't know if this matters but it seems like an odd choice to be making here. |
| const deserialize = (serialized) => { | ||
| const ethObj = new EthAccount(serialized) | ||
|
|
||
| const deserialized = { |
There was a problem hiding this comment.
nice! and now I can grok what this object even is
util/createResolver.js
Outdated
| node === null) { | ||
| return | ||
| } | ||
| //console.log('vmx: node:', node) |
| callback(null, paths) | ||
| }) | ||
| } else { | ||
| // other nodes link by hash |
There was a problem hiding this comment.
That's actually intentionally. I still try to figure out how to format "if/else" comments properly.
My preferred style is:
// Some case
if {
}
// And in case of xyz
else {
}
But that doesn't work with our linting rules.
// Some case
if {
}
else {
// And in case of xyz
}
Also feels a bit weird, as the comment is not about the subsequent line, but about the whole else.
Hence I went for
if {
// Some case
}
else {
// And in case of xyz
}
for consistency.
|
@rvagg About this "ommers" thing. I just didn't want to break backwards compatibility, it's also in the old code: js-ipld-ethereum/eth-block/index.js Line 40 in 4eaa791 |
vmx
force-pushed
the
ipld-format-cleanup-other
branch
from
12ca4bb to
dc0b8d7
Compare
|
New forced push (I always forget that GitHub doesn't notify on new pushes). |
|
yeah, I know the ommers thing is old, just wondering if anyone listening to this has context, just out of interest. This lgtm |
BREAKING CHANGE: The API is now async/await based There are numerous changes, the most significant one is that the API is no longer callback based, but it using async/await. If you want to access the original JavaScript Ethereum object, it is now stored in a property called `_ethObj`. So if you e.g. called `deserialized.hash()`, you now have to call `deserialized._ethObj.hash()`. For the full new API please see the [IPLD Formats spec]. [IPLD Formats spec]: https://github.com/ipld/interface-ipld-format
vmx
force-pushed
the
ipld-format-cleanup-other
branch
from
dc0b8d7 to
34263b6
Compare
|
I just force-pushed a better commit message. |
BREAKING CHANGE: The API is now async/await based
There are numerous changes, the most significant one is that the API
is no longer callback based, but it using async/await.
For the full new API please see the IPLD Formats spec.