Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support array for serviceEndpoint #1200

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

nikolockenvitz
Copy link

related to #1198 @csuwildcat

should the breaking change notice be part of the merge commit?

OR13
OR13 previously approved these changes Sep 20, 2022
@csuwildcat
Copy link
Member

@thehenrytsai looks like a good one to pull in, given the DID Comms enablement

troyronda
troyronda previously approved these changes Sep 21, 2022
Copy link
Collaborator

@troyronda troyronda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to support an array of service endpoints.

BenjaminMoe
BenjaminMoe previously approved these changes Sep 23, 2022
@csuwildcat
Copy link
Member

@nikolockenvitz I would add the breaking change to the commit, yes

Copy link
Collaborator

@thehenrytsai thehenrytsai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

lib/core/versions/latest/DocumentComposer.ts Show resolved Hide resolved
@@ -4,5 +4,5 @@
export default interface ServiceModel {
id: string;
type: string;
serviceEndpoint: string | object;
serviceEndpoint: string | object | Array<string | object>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably no need for this change, array is an object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +350 to +366
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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thehenrytsai, I updated ion-sdk (decentralized-identity/ion-sdk#30) as well

@nikolockenvitz
Copy link
Author

@thehenrytsai can you please check whether all your concerns are addressed or point out required changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants