feat!: Updated resolve method signature #82
Conversation
| if (!resolver) { | ||
| throw new Error(`Unsupported DID method: '${parsed.method}'`) | ||
| } | ||
| return this.cache(parsed, () => resolver(parsed.did, parsed, this, options)) |
There was a problem hiding this comment.
I believe this can be made backward compatible if the result is inspected.
It should be easy to check for result.hasOwnProperty('didResolutionMetadata'), didDocument, and/or didDocumentMetadata since none of these fields are part of the DIDDocument return type of resolvers.
There was a problem hiding this comment.
However, the old DIDDocument type is not compatible with the new one and would need to be converted somehow. Also all baseXX encoding are not supported anymore, so we would need to do some conversion there.
There was a problem hiding this comment.
right, but there is no check here either for the correct DIDDocument format coming from the method implementations. the result is simply bubbled up.
The only check is done by the IDE/transpiler in case the method implementation is also using typescript, but that is not really a guarantee.
There was a problem hiding this comment.
Sure, but what are you suggesting instead?
There was a problem hiding this comment.
To inspect the result and wrap it if it's legacy:
| return this.cache(parsed, () => resolver(parsed.did, parsed, this, options)) | |
| return this.cache(parsed, () => { | |
| let result = null | |
| try { | |
| result = await resolver(parsed.did, parsed, this, options) | |
| } catch (e) { | |
| return { | |
| didResolutionMetadata: { | |
| error: 'notFound' // or perhaps another error code | |
| message: e.toString() // This is not in spec, nut may be helpful | |
| }, | |
| didDocument: null, | |
| didDocumentMetadata: {} | |
| } | |
| } | |
| if (!result.hasOwnProperty('didResolutionMetadata') | |
| && !result.hasOwnProperty('didDocumentMetadata') | |
| && !result.hasOwnProperty('didDocument')) { | |
| return {didResolutionMetadata: {}, didDocument: result, didDocumentMetadata: {}} | |
| } else { | |
| return result | |
| } | |
| }) |
There was a problem hiding this comment.
Don't like this because it will confuse the method signature of the DIDResolver to return either DIDResolutionResult or DIDDocument.
|
Pushed an update with support for letgacy did resolvers. |
| if (doc !== null) { | ||
| cache.set(parsed.didUrl, doc) | ||
| const result = await resolve() | ||
| if (result.didResolutionMetadata?.error !== 'notFound') { |
There was a problem hiding this comment.
Why do you think errors should be cached?
There was a problem hiding this comment.
I just don't see why notFound should be treated differently from when something is found. Doesn't matter too much though.
There was a problem hiding this comment.
no, I mean why cache ANY error results?
# [3.0.0](2.2.0...3.0.0) (2021-03-02) ### Features * update resolve method signature to match did core spec ([#82](#82)) ([313e685](313e685)), closes [#79](#79) ### BREAKING CHANGES * the `Resolver` and `DIDResolver` data types have been upgraded. To use a legacy resolver that simply returns a `DIDDocument`, please register it under the `options.legacyResolvers` constructor param
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR changes the resolve method signature according to #79
It also updates the DIDDocument interface to be compliant with the latest did-core spec.
Question about the last test: Why do we not want to cache null results, i.e.
notFound? Seems to me that we should cache that just the same way we cache any other result?Skipped this test for now.
The readme for this repo will need a revamp.