Skip to content
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

Use newer js-ipfs proxy libraries #288

Merged
merged 8 commits into from
Oct 13, 2021
Merged

Use newer js-ipfs proxy libraries #288

merged 8 commits into from
Oct 13, 2021

Conversation

icidasset
Copy link
Contributor

@icidasset icidasset commented Sep 17, 2021

Gets rid of our ipfs-message-port-* forks and versions the ipfs worker, because said libraries aren't backwards compatible (due to CID implementation changes).

How the versioning works:

  • We keep the old ipfs.html path, so older webnative clients know where to look.
  • We do a HTML redirect to the older version by using an ipfs hash.
  • Newer webnative clients will load in ipfs/v2.html which works as before, but with the new message-port libraries.

Will update to stable version which comes out soon.

Closes #250

@matheus23
Copy link
Contributor

But maybe this is some inconsistency with all the different CID objects/classes floating around. We mostly use strings, there's the CID class from the cids package, then the one from multiformats and possibly another one from the ipfs packages (depending on the version).

This might be due to js-ipfs's change to a new CID library. They've now moved on to the multiformats/cid as representation for CIDs everywhere and deprecated the cids package. We'd need up update js-ipfs generally for that though.
I've done that in the wnfs2 branch and I think I could backport that change for v1 as well. Should I do that?

@matheus23
Copy link
Contributor

matheus23 commented Sep 27, 2021

since js-ipfs doesn't do pinning anyway

I think you ment to say "doesn't garbage collect", right?

@icidasset
Copy link
Contributor Author

I've done that in the wnfs2 branch and I think I could backport that change for v1 as well. Should I do that?

Nah it's fine, we'll power through until your wnfs2 branch is merged 😜

I think you ment to say "doesn't garbage collect", right?

Yeah, for me pinning is synonymous to "don't garbage collect".

@@ -17,5 +17,5 @@ export const setup = {
user: "fission.name"
},

shouldPin: true,
shouldPin: false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just remove this now, right?
I'm not 100% sure. There might be a use-case for pinning for users who're providing their own IPFS instance.
... But at the same time I think there's noone who's actually doing that. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't hurt to keep it around I guess? In case js-ipfs does turn on GC, like how it's supposed to work 😅

return new dagPb.DAGLink(raw.Name, raw.Tsize, raw.Hash)
const h = raw.Hash
const c = new OldCID(h.version, h.code, h.multihash.bytes)
return new dagPb.DAGLink(raw.Name, raw.Tsize, c)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a bit dumb, but I didn't want to put too much work in here, since this is getting replaced soon anyway by WNFS v2.

fs.mkdirSync(dir)

const memoryDs = new MemoryDatastore()
const memoryBs = new MemoryBlockstore()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matheus23 Bit of double effort here, but needed to replace this to get the tests working with the new ipfs-core.

@icidasset icidasset marked this pull request as ready for review October 4, 2021 18:45
@icidasset
Copy link
Contributor Author

Ready for review. Goes hand in hand with this lobby PR: fission-codes/auth-lobby#92 Which I'll need to deploy to staging to get all the tests passing.

@@ -13,7 +13,7 @@ export async function loadCAR(filepath: string, ipfs: IPFS): Promise<{ roots: CI
const blockIterator = await CarBlockIterator.fromIterable(inStream)
for await (const block of blockIterator) {
cids.push(block.cid)
await ipfs.block.put(block.bytes, { cid: block.cid })
await ipfs.block.put(block.bytes, { version: 1, mhtype: "sha2-256" })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code has issues, because it'll strip away the IPLD codec from all CIDs and turn all blocks to "raw" blocks implicitly (this code is probably from the wnfs2 branch).

We should totally get rid of this whole thing and just use the newly implemented ipfs.dag.import api.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aight! I'll see if I can use that import function 👀

@matheus23
Copy link
Contributor

The tests are failing because, ... uh. They depend on staging infrastructure lol.
We need fission-codes/auth-lobby#92 to land in staging to get this to test successfully.

Copy link
Contributor

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally no more forks 🤩 🎉 🥳 AND updated js-ipfs :) 👍

@matheus23 matheus23 merged commit cad228c into main Oct 13, 2021
@matheus23 matheus23 deleted the icidasset/no-pin branch October 13, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installation issue with yarn2
2 participants