IPFS plugin separation of concerns#930
Merged
dOrgJelli merged 30 commits intoprealpha-devfrom Jun 27, 2022
Merged
Conversation
pileks
changed the title
pileks
requested review from
dOrgJelli,
krisbitney,
namesty and
nerfZael
as code owners
nerfZael
requested changes
Jun 22, 2022
cbrzn
reviewed
Jun 23, 2022
cbrzn
reviewed
Jun 23, 2022
Contributor
cbrzn
left a comment
cbrzn
left a comment
There was a problem hiding this comment.
Awesome stuff ser 🔥
Feel free to ignore my code styles suggestions :-D
| schema: ./src/schema.graphql | ||
| import_redirects: | ||
| - uri: "ens/uri-resolver.core.polywrap.eth" | ||
| schema: ../../../../core-interfaces/uri-resolver/src/query.graphql |
Contributor
There was a problem hiding this comment.
Let's remove the query word and rename the file to schema.graphql
Contributor
Author
There was a problem hiding this comment.
This is referenced in multiple PRs right now - If it's okay with you, I'd do this as part of an end-of-PRs "cleanup PR" which would redo some of the folder structure, and change naming. We should also think about building the interface rather than referencing directly from src - something I've done in the FS MR (#912) and this one for their respective interfaces.
…lugin-separation-of-concerns
… multiformats package
URI resolver test fixes
…lugin-separation-of-concerns
nerfZael
previously approved these changes
Jun 23, 2022
…lugin-separation-of-concerns
dOrgJelli
reviewed
Jun 27, 2022
dOrgJelli
approved these changes
Jun 27, 2022
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
4 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.
Partially closes #622 and #714 (IPFS part)
After discussion with @nerfZael the following was done:
Introduced an interface for IPFS-like plugins
Extracted uri-resolver interface implementation from IPFS plugin into its own
ipfs-uri-resolver-plugin-jspluginipfs-plugin-jsnow contains only basic IPFS-related functions, with theipfs-uri-resolver-plugin-jsdepending on theipfs-plugin-js.Introduced a new build command for interfaces that follow the @web3api/*-interface pattern and added it to the main build command - same as #912
Possible circular dependency noticed:
Interfaces require CLI to be built, which results in a
CLI -> Plugins -> Interfaces -> CLIcircular dependency. Advice on how to proceed is very much appreciated.Remaining work:
ipfs-uri-resolver-plugin-jsipfs-plugin-js<- I noticed these were missing@dOrgJelli We commented on moving the resolver plugins into a separate folder - I would like to do this once we finish up the separation of concerns work so that we move all 3 resolver plugins as part of a single PR. Hope that's okay.
Advice/feedback would be much appreciated!