Support array for serviceEndpoint#1200
Support array for serviceEndpoint#1200nikolockenvitz wants to merge 3 commits intodecentralized-identity:masterfrom
Conversation
nikolockenvitz
requested review from
OR13,
csuwildcat,
thehenrytsai,
tplooker,
troyronda and
xinaxu
as code owners
|
@thehenrytsai looks like a good one to pull in, given the DID Comms enablement |
troyronda
left a comment
troyronda
left a comment
There was a problem hiding this comment.
Makes sense to support an array of service endpoints.
|
@nikolockenvitz I would add the breaking change to the commit, yes |
thehenrytsai
left a comment
thehenrytsai
left a comment
There was a problem hiding this comment.
Sorry for taking so long to get back. The PR looks good overall, just a couple of comments for this PR to actually take effect.
| id: string; | ||
| type: string; | ||
| serviceEndpoint: string | object; | ||
| serviceEndpoint: string | object | Array<string | object>; |
There was a problem hiding this comment.
Probably no need for this change, array is an object.
There was a problem hiding this comment.
That's true, I just thought it might be better to understand if it's explicit. But I can also revert this change if you like.
| for (const serviceEndpoint of serviceEndpointValueAsArray) { | ||
| // serviceEndpoint itself must be URI string or non-array object | ||
| if (typeof serviceEndpoint === 'string') { | ||
| const uri = URI.parse(serviceEndpoint); | ||
| if (uri.error !== undefined) { | ||
| throw new SidetreeError( | ||
| ErrorCode.DocumentComposerPatchServiceEndpointStringNotValidUri, | ||
| `Service endpoint string '${serviceEndpoint}' is not a valid URI.` | ||
| ); | ||
| } | ||
| } else if (typeof serviceEndpoint === 'object') { | ||
| // Allow `object` type only if it is not an array. | ||
| if (Array.isArray(serviceEndpoint)) { | ||
| throw new SidetreeError(ErrorCode.DocumentComposerPatchServiceEndpointCannotBeAnArray); | ||
| } | ||
| } else { | ||
| throw new SidetreeError(ErrorCode.DocumentComposerPatchServiceEndpointMustBeStringOrNonArrayObject); |
There was a problem hiding this comment.
We will need the same change in ion-sdk. Totally get it if you don't have time to make those changes in ion-sdk, so the PR will suffice to be approved if you can file an issue in the ion-sdk repo to make the same changes! Thanks!
There was a problem hiding this comment.
Happy to implement this in ion-sdk as well. I will still create an issue as well, as I am not yet 100% sure when I will find time for the implementation. So I will open the issue, once this PR is merged, right?
There was a problem hiding this comment.
Hi @thehenrytsai, I updated ion-sdk (decentralized-identity/ion-sdk#30) as well
nikolockenvitz
dismissed stale reviews from BenjaminMoe, troyronda, and OR13
via
4012c5f
|
@thehenrytsai can you please check whether all your concerns are addressed or point out required changes? |
6 participants
related to #1198 @csuwildcat
should the breaking change notice be part of the merge commit?