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

chore(blog): websocket series part 2 #225

Merged
merged 13 commits into from
Apr 21, 2021
Merged

Conversation

derberg
Copy link
Member

@derberg derberg commented Apr 7, 2021

Description

See also: asyncapi/spec#253

@netlify
Copy link

netlify bot commented Apr 7, 2021

Deploy preview for asyncapi-website processing.

Building with commit 84ebdb0

https://app.netlify.com/sites/asyncapi-website/deploys/607ea7c000ddcf0007194851

@derberg derberg marked this pull request as ready for review April 15, 2021 13:06
@derberg derberg requested a review from fmvilas April 15, 2021 13:06
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Love the artcile. Finally a much-needed blog post about WebSockets 👏

pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
pages/blog/websocket-part2.md Outdated Show resolved Hide resolved
Comment on lines 317 to 320
- errorMessage
- required:
- channelID
- channelName
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make properties mutually exclusive, as you say above. Only the fact that it's required or not but all the three properties can coexist in an object as per this definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

you are rights, I should have not specified too. Will add

- channelName
```

I managed to get a structure that will be nicely rendered in the UI. Even code generation will work well. The problem is that I failed with DRY rule and have most of the structure repeated, which makes the solution error-prone.
Copy link
Member

Choose a reason for hiding this comment

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

You can still extract a big part of this schema. For instance, subscription, status, pair, reqid, and event are the same so they can be extracted.

Also, another way to do this is to use allOf. You define a schema that contains what's common between both schemas and then define the two as "allOf" of commonPart + specificPart. Less error prone.

Copy link
Member Author

Choose a reason for hiding this comment

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

regarding allOf how would I specify that you have object A or B in the end. allOf means all, right? so all of common + specific, but the specific part has two different versions.

Copy link
Member

Choose a reason for hiding this comment

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

ErrorMessage = allOf(common, specific)
RegularMessage = allOf(common, specific)

FinalMessage = oneOf(ErrorMessage, RegularMessage)

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right mate, loving it, no repetition too:

    subscriptionStatus:
      type: object
      oneOf:
        - allOf:
            - properties:
                errorMessage:
                  type: string
              required:
                - errorMessage
            - $ref: '#/components/schemas/subscriptionStatusCommon'
        - allOf:
            - properties:
                channelID:
                  type: integer
                  description: ChannelID on successful subscription, applicable to public messages only.
                channelName:
                  type: string
                  description: Channel Name on successful subscription. For payloads 'ohlc' and 'book', respective interval or depth will be added as suffix.
              required:
                - channelID
                - channelName
            - $ref: '#/components/schemas/subscriptionStatusCommon'
    subscriptionStatusCommon:
      type: object
      required:
         - event
      properties:
        event:
          type: string
          const: subscriptionStatus
        reqid:
          $ref: '#/components/schemas/reqid'
        pair:
          $ref: '#/components/schemas/pair'
        status:
          $ref: '#/components/schemas/status'
        subscription:
          required:
            - name
          type: object
          properties:
            depth:
              $ref: '#/components/schemas/depth'
            interval:
              $ref: '#/components/schemas/interval'
            maxratecount:
              $ref: '#/components/schemas/maxratecount'
            name:
              $ref: '#/components/schemas/name'
            token:
              $ref: '#/components/schemas/token'

and it is supported by our playground 💪🏼
I'll update the article,

Copy link
Member

Choose a reason for hiding this comment

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

Only suggestion is to create different schemas for each allOf. This is hard to read 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

yup 😄
have a look now, I updated all references, looks good in docs, super cool stuff I think

Screenshot 2021-04-20 at 11 56 06

@derberg derberg requested a review from fmvilas April 20, 2021 10:25
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Great stuff mate! 👏

@derberg derberg merged commit 0de1c28 into asyncapi:master Apr 21, 2021
@derberg derberg deleted the websocket2 branch April 21, 2021 08:43
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.

2 participants