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

chore: upgrade ipfs to 0.55.x #661

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

chore: upgrade ipfs to 0.55.x #661

wants to merge 8 commits into from

Conversation

achingbrain
Copy link
Member

Updates to the latest ipfs release. Some extra build config was
needed because it uses optional chaining.

This is supported out of the box by the version of acorn used by
webpack 5 but vue is currently using webpack 4 so some extra config
was necessary.

Also uprades to-buffer - this now returns Uint8Arrays instead
of the node Buffer built in, which means you can't call
.toString(encoding) on it any more, instead you need to use the
global js TextDecoder class.

@achingbrain
Copy link
Member Author

Do I have to create a fleek account to see why CI fails?

@zebateira
Copy link
Contributor

zebateira commented May 13, 2021

@achingbrain I'm afraid so.
I checked and it's failing on lesson 7 of the Regular Files API tutorial.

@achingbrain
Copy link
Member Author

Ah, right:

return bufferedContents.toString()

probably needs to change to:

return new TextDecoder().decode(bufferedContents)

@zebateira zebateira linked an issue May 14, 2021 that may be closed by this pull request
Zé Bateira added 4 commits June 14, 2021 12:56
Updates to the latest ipfs release. Some extra build config was
needed because it uses optional chaining.

This is supported out of the box by the version of acorn used by
webpack 5 but vue is currently using webpack 4 so some extra config
was necessary.

Also uprades `to-buffer` - this now returns `Uint8Array`s instead
of the node `Buffer` built in, which means you can't call
`.toString(encoding)` on it any more, instead you need to use the
global js `TextDecoder` class.
@zebateira
Copy link
Contributor

zebateira commented Jun 14, 2021

@terichadbourne ready for your review.
I'm not sure about the description of the new TextDecoder().decode(), but you can read through it and see if it's ok.

Thank you @achingbrain for working on this! 🙏

@terichadbourne
Copy link
Member

Thank you both for all of your help with this. I just went through each lesson and compared the output of the solution code to the lesson contents about what the method is supposed to return. There were a few methods where this had changed, eg as pointed out in #691), so I updated the lesson text to match the output I actually see.

@achingbrain Could I ask you to do me the favor of reviewing my 4 recent commits (all small) to ensure you agree with the way I've described what the user should now expect? In the last commit, there were a few cases where I didn't know how to properly describe what a field meant (or the unit it was presented in and would love your help. (I add TODOs in the code.)

The last TODO in the Regular Files API tutorial is a spot where we tell them that the result of ipfs.get will include a field content that contains a Buffer but only if the type is file. What I actually see in the results suggests that when it's a file, it returns an empty object, and when it's a directory it omits the field.

image

Thanks in advance for your help on this!

@terichadbourne
Copy link
Member

@achingbrain Just a ping here - could you find time to review my last 4 commits and make sure they're accurate and answer the question in my comment above? Thanks!

- `mode`: the UnixFS mode as a Number
- `mtime`: an object with numeric `secs` and `nsecs` properties
Copy link
Member Author

Choose a reason for hiding this comment

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

This field will be present if found in the DAG so it's probably worth keeping it here with a note that it's an optional property.

That is, mode gets a default value depending on if the entry is a file or directory, but mtime does not.

@achingbrain
Copy link
Member Author

The last TODO in the Regular Files API tutorial is a spot where we tell them that the result of ipfs.get will include..

It's probably worth revisiting this exercise. In ipfs@0.57.0 the output of ipfs.get changed from sort of being a bit like a recursive ipfs.ls to outputting tarballs in line with the CLI/go-IPFS HTTP API.

ipfs.get also had a fairly significant caveat in that over HTTP the API has always outputted a tarball which was then unpacked by the client to present the same API as core, which means that if you were using the HTTP API client you'd have to consume the contents of a file before you could move on to the next entry, which is a terribly leaky abstraction and not a nice thing to have to explain to people.

There's now a clear reason to use ipfs.get vs ipfs.ls as the former exports a UnixFS file/directory structure in an archive and the latter lists metadata about a file/directory and would be used in conjunction with ipfs.cat to get file contents and ipfs.ls to descend further into the directory structure.

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.

Update ProtoSchool to use js-ipfs 0.55.0
3 participants