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

Allow servers and channels to be defined inside components #660

Closed
smoya opened this issue Nov 30, 2021 · 14 comments · Fixed by #681
Closed

Allow servers and channels to be defined inside components #660

smoya opened this issue Nov 30, 2021 · 14 comments · Fixed by #681
Assignees
Labels
🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md)

Comments

@smoya
Copy link
Member

smoya commented Nov 30, 2021

Context

This is part of the RFC #618 (comment).
In an effort around reducing the scope of such RFC, it has been split into several proposals so they can move forward independently.

The problem

Users can't define servers and channels as components. Meaning this has an impact on reusability.
In the scenario of #654 and #658, reusing components is more than a requirement.
This will allow users to reference both servers and channels so the following will be now valid:

servers:
  production:
    $ref: '#/components/servers/myserver'
channels:
  some/events:
    $ref: '#/components/channels/myChannel'
components:
  servers:
    myserver:
      url: "http://localhost:5000/ws"
      protocol: ws
  channels:
    myChannel:
      description: "mychannel"

Suggestion

To allow servers and channels to be defined inside components.
As far as I see, this won't be considered as breaking change.

Depends on #650

@smoya smoya added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Nov 30, 2021
@smoya smoya changed the title Allow servers and channels to be defined inside components Allow servers and channels to be defined inside components Nov 30, 2021
@smoya smoya self-assigned this Dec 1, 2021
@smoya
Copy link
Member Author

smoya commented Dec 1, 2021

I'm championing this one if no one is against it.

@jonaslagoni
Copy link
Member

I think you have to include part of #650 so that servers and channels can be referenced, otherwise there is no need to define it in components 🙂

@smoya
Copy link
Member Author

smoya commented Dec 2, 2021

Some work has been done now in order to properly illustrate the change. That doesn't mean this should be accepted.

Pending: work on Parser-js (if there is any)

@smoya
Copy link
Member Author

smoya commented Dec 7, 2021

I'm gonna ping some people I think might be interested in this. Please do not feel you have to review this, just to raise some awareness :) Thanks!

cc @fmvilas @GeraldLoeffler @damaru-inc @jessemenning @magicmatatjahu

@fmvilas
Copy link
Member

fmvilas commented Dec 8, 2021

Can we clarify that this is depending on #650? Otherwise, as Jonas said, it's useless, right?

@smoya
Copy link
Member Author

smoya commented Dec 8, 2021

Can we clarify that this is depending on #650? Otherwise, as Jonas said, it's useless, right?

The current changes in both opened PRs (as per today) consider #650. For example, asyncapi/spec-json-schemas#133 adds $ref field to the server object.
I added a link to #650 in the description of this Issue as this is what I guess you are missing.

@smoya
Copy link
Member Author

smoya commented Dec 16, 2021

Parser-JS PR is now a thing asyncapi/parser-js#430 🎉

@derberg
Copy link
Member

derberg commented Dec 21, 2021

my point of view here is that even if this would not be driven by the RFC about publish/subscribe, this PR would still make sense. Mainly because of Simplicity and consistency over expressiveness and terseness principle.

In a single document, referencing server or channel to components doesn't make much sense, but we know that people do share data between asyncapi documents, just look at https://github.com/asyncapi/spec/tree/master/examples/social-media. So yeah, current problem is that you can reference server and need to duplicate it.

And anyway for consistency I think it is good that all root objects can be in components.

Imho this is a good candidate for stage 3, Draft, we just need to review and approve implementation in schema and parser.
My only request for this PR would be that once we update release branch, this PR should showcase reusability in this new example about social media as much as possible.

@fmvilas thoughts?

@smoya
Copy link
Member Author

smoya commented Dec 22, 2021

this PR should showcase reusability in this new example about social media as much as possible.

I've been moving forward with this and, unfortunately, the only thing it can be moved to components is one of the servers. See the diff here.
The reason for not moving the rest of servers and channels is because some field change, such as the descriptiion and, as per today, our reference and bundling system does not support fields to be overridden (We follow JSON Reference).

@smoya
Copy link
Member Author

smoya commented Dec 28, 2021

As @derberg requested, I've updated the social media example. The changes has been added to the existing PR #665

@derberg
Copy link
Member

derberg commented Jan 3, 2022

@fmvilas from my POV this one can be promoted to Stage 2: Draft as all 3 PRs are ready:

any only one small thing must be clarified in the Parser-related PR and that is it.

@smoya
Copy link
Member Author

smoya commented Jan 7, 2022

@fmvilas from my POV this one can be promoted to Stage 2: Draft as all 3 PRs are ready:

any only one small thing must be clarified in the Parser-related PR and that is it.

And just for keeping this issue up-to-date, the thing to be clarified on the Parser-js PR is done and waiting for approval.

@derberg
Copy link
Member

derberg commented Jan 12, 2022

@fmvilas from my POV this one is ready to merge into release branches, I approved all PRs mentioned by @smoya in the comment above. So have a look, and accept if you are ok and let us promote it to stage 3 and merge these PRs one by one (I can do it)

@fmvilas
Copy link
Member

fmvilas commented Jan 12, 2022

Yup, agree this can be promoted to stage 3. Approved the other PRs as well. Great stuff, @smoya!

@fmvilas fmvilas added 🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md) and removed 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) labels Jan 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁 Accepted (RFC 3) RFC Stage 3 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants