IPFS Plugin - move config into envs + method options #1060
Conversation
pileks
requested review from
dOrgJelli,
krisbitney,
nerfZael and
namesty
as code owners
…fig-env # Conflicts: # packages/js/client/src/__tests__/utils/getClientWithEnsAndIpfs.ts # packages/js/client/src/default-client-config.ts # packages/js/plugins/uri-resolvers/ens-resolver/src/__tests__/e2e.spec.ts # packages/js/plugins/uri-resolvers/ipfs-resolver/src/__tests__/e2e.spec.ts
| @@ -40,7 +40,8 @@ export class IpfsResolverPlugin extends Module<NoConfig> { | |||
| { | |||
| cid: `${args.path}/${manifestSearchPattern}`, | |||
| options: { | |||
| timeout: 5000, | |||
There was a problem hiding this comment.
By removing the default timeout values, it looks like the timeouts will default to 0. I don't know much about IPFS timeouts. Is 0 unlimited timeout? Is this a good default value? Or should we keep default timeout values somewhere? For example, we could have default timeouts in the ipfs uri resolver like this.env.timeouts?.tryResolveUri ?? 5000, or we could add a default env in the default config. What do you think?
There was a problem hiding this comment.
Yes, a zero is an unlimited timeout. We could go with something simple like a single defaultTimeout which we could use for all actions? Seems like an okay solution, and we don't necessarily need to have a full default local env (especially since we wouldn't be defining all env defaults, only certain fields).
There was a problem hiding this comment.
That makes sense to me. I don't know the best way to handle this. I just wanted to bring up the question. I think the default behavior should be that it "just works" without having to configure timeouts, and timeout configuration should be optional. If unlimited timeouts won't cause any issues (like an app hanging, waiting for a response that will never come) then that would be fine.
| """ | ||
| Determines whether the plugin should try to resolve a file (check for its existence) or immediately get the file | ||
| """ | ||
| skipCheckIfExistsBeforeGetFile: Boolean, |
There was a problem hiding this comment.
Brainstorming other names
skipCheckIfExists
skipFileCheck
skipCheckBeforeGetFile
I think the name you have already is long but clear and informative. If you decide to change it, my favorite alternative is skipCheckIfExists because it matches the timeout name.
There was a problem hiding this comment.
I think skipCheckIfExists sounds best too! Will go with that one.
Including Env in the interface would assume too much regarding the implementation itself - IMO we should not include it in the interface schema. |
Want to complete before merge:
fallbackProvidersfallbackProvidersbe part of the IPFS interface specification? Seems like the answer would be “no”, because this would mean we are assuming that all implementations of the IPFS plugin should use fallback providers.fallbackProvidersinto theOptionstype of the Ipfs interfaceskipCheckIfExistsBeforeGetFilein IPFS Resolver Plugin envproviderssystem instead of having aproviderandfallbackProvidersEnvin IPFS Interface schema?