-
Notifications
You must be signed in to change notification settings - Fork 23
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!: replace ipld-dag-pb
with @ipld/dag-pb
#330
Conversation
|
||
const { CID, hasher, bytes } = multiformats | ||
|
||
export const sha256 = hasher.from({ |
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.
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.
import { sha256 } from 'multiformats/hashes/sha2'
if the docs are wrong, a PR would be appreciated, but the export is there and we actively use it in many places
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.
That import doesn't even work :(
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.
what runtime or bundler is this using? perhaps something needs to be updated here to support proper export maps?
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.
[email protected] is probably the culprit here, I don't think they'd nailed down their export maps functionality quite right back then but I don't know why this import specifically would fail tbh
but I bet upgrading react here would be a major pain too! so go with whatever works I suppose.
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'm assuming this has something to do with migrating js-ipfs stuff to esm, but the GUI packages all still being cjs
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.
self-review completed.. trivial errors and unnecessary comments should be gone
I don't understand the Do we need to update runtimes used in CI here or something? |
@SgtPooki maybe I missed it, but is there any reason to not use ipld/js-blockcodec-to-ipld-format to convert the new format to the old format (and therefore be supported here) like it was done in #319? |
@RangerMauve ping on the export maps / react / tooling discussion - here's a pain-point I think, see discussion inline about sha256. |
Gonna look into this early next week as part of the ESM migration stuff. |
I would say the main reason is that it leaves our code in a state of perpetual deprecation. I'll try it just to keep the code-changes down, but we still need a better story for moving forward. Also, Instead of us maintaining something like https://github.com/ipld/js-blockcodec-to-ipld-format (last update 16 months ago), creating a tool that migrates old code to the new code would be much better. |
it's a jsdom thing. I believe I have a workaround. See jsdom/jsdom#2524 |
Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
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.
Codewise, it looks good to me. However, I would like to actually try running this, perhaps using https://github.com/ipld/explore.ipld.io to make sure it works properly. Sadly, I did not manage to make it work with either npm link
, or npx link
. Besides the storybooks, how did you try this out?
I just used storybook. I just attempted https://github.com/ipld/explore.ipld.io too, but that will require updating ipld-dag-pb in that repo as well. Getting it to work in that repo just to prove that it works here (which it does, fine, in storybook) seems excessive. If we have to do that for every change needed to resolve ipfs/ipfs-webui#1965 then that issue will be the death of webui. |
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.
You're right, let's do it!
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
ipld-dag-pb
with @ipld/dag-pb
@hacdias marked PR title with |
Great. I'm fine to merge this. |
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
FYI: Much of this was me scrambling in the dark without knowing 1/4th of what I'm doing. IDK what:1. the correct interface is supposed to be (the docs and source-code of dependencies are out of date or otherwise incorrect)2. what exactly ipld/pb/cbor is supposed to do, especially in this repo3. Any and everything else you may assume I knowWith that said,comments & feedback that are typically given noobs and to the otherwise clueless are more than welcome and honestly expected.fixes #329
I could use some eyes from the experts in this space. @achingbrain @lidel @mikeal