Use native ipfs types and remove hand-written ones#243
Merged
Conversation
Contributor
Author
|
I've updated dependencies in this PR. I think merging this will also close most of the dependabot PRs. |
icidasset
approved these changes
Jun 2, 2021
Contributor
icidasset
left a comment
icidasset
left a comment
There was a problem hiding this comment.
Few questions, looks great otherwise!
| "ipfs-core": "^0.6.1", | ||
| "cids": "^1.1.6", | ||
| "fission-bloom-filters": "1.7.0", | ||
| "ipfs-core": "^0.7.0", |
Contributor
There was a problem hiding this comment.
Shouldn't this be in devDependencies?
| import { DAGNode, RawDAGNode, DAGLink, RawDAGLink } from './types' | ||
| import dagPB, { DAGLink, DAGNode } from 'ipld-dag-pb' | ||
| import type { GetResult } from 'ipfs-core-types/src/dag' | ||
| import { CID } from 'ipfs-message-port-client/src/block' |
Contributor
There was a problem hiding this comment.
Is this the same CID type as from the cids library?
Contributor
Author
There was a problem hiding this comment.
Nope. Super annoying that it isn't. Not sure what's up with that in the js-ipfs ecosystems right now 🤔
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This will make webnative use the typescript types that were now developed by ipfs (ipfs/js-ipfs#2945).
This in turn makes the types more accurate, as it seems they've become somewhat outdated. E.g. see this line in the dashboard for one of the cases we've gotten it slightly wrong.
Test plan (required)
Unfortunately it's not a pure refactor: On the public side, I've had to transform the
FileContenttype intoImportCandidate(from ipfs). This makes sense, because some of theFileContenttypes wouldn't be supported to be added on the public filesystem side.This changes behavior, but I decided that in this case it makes sense to fix the implementation and adjust it to the types instead of adjusting the types. This is something I'm not 100% sure about, so please discuss if you have some ideas.
Since I've changed behavior, we should test some apps, including drive, the dashboard & maybe flatmate (or something else using webnative-elm).