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

Type-Check CID #365

Merged
merged 4 commits into from
Apr 1, 2022
Merged

Type-Check CID #365

merged 4 commits into from
Apr 1, 2022

Conversation

appcypher
Copy link
Contributor

Summary

This PR implements the following features

  • Type-checks CID for better error message

cids have been updated to use the multiformat-based CID class but old code still use string encoded formats which leads to vague errors. This PR adds a check that returns a clear error message when a cid parameter isn't an object.

Closing issues

Fixes #345

@appcypher appcypher changed the title Type check CID in public api Type-Check CID Mar 30, 2022
@@ -164,6 +164,8 @@ export class FileSystem {
* Loads an existing file system from a CID.
*/
static async fromCID(cid: CID, opts: FileSystemOptions = {}): Promise<FileSystem | null> {
if (!(cid instanceof CID)) throw new Error("Expected a CID object")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry for throwing you into JS/TS land which is horrible :P

instanceof is dangerous in JS, because if for whatever reason you'll end up with a duplicated copy of the multiformats library in your bundle, their CID classes are not instanceof-compatible with each other.

Here's an issue comment that shows the desired pattern to use instead: multiformats/js-cid#111 (comment)

I know, this is an annoyingly small thing and you want to get working on something bigger, it's just for you to get familiar with different parts of our toolchain + code. (Almost done 🎉)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It's fine. You have been really helpful. 😀

@appcypher appcypher marked this pull request as ready for review March 31, 2022 13:37
src/ipfs/basic.ts Outdated Show resolved Hide resolved
@appcypher appcypher added the no changelog Attached to PRs which shouldn't require changelog changes label Mar 31, 2022
@appcypher
Copy link
Contributor Author

For context and as discussed with @icidasset, the error we get comes from CID being sent via postMessage to a decoding piece that assumes CIDs are of CID class instance (not strings). So the ideal place to do a check is right before the CID is sent since there are other areas in the code that handle a CID multiform nature.

@matheus23
Copy link
Contributor

Can we make this cid = typeCheckCID(cid), where typeCheckCID returns what CID.asCID returns if its non-falsy?
This way we're automatically converting between a couple of CID types in some edge cases.

@appcypher
Copy link
Contributor Author

Good to go? @icidasset @matheus23

Copy link
Contributor

@icidasset icidasset left a comment

Choose a reason for hiding this comment

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

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.

🎉

@appcypher appcypher merged commit 101615d into main Apr 1, 2022
@appcypher appcypher deleted the appcypher/type-check-cid branch April 1, 2022 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog Attached to PRs which shouldn't require changelog changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CID conversion error
3 participants