feat: make channels optional

[smoya]

Description

Part of asyncapi/spec#661.

This PR removes channels from the required fields on the root of the document, meaning channels is not required to be present anymore.

Related issue(s)
asyncapi/spec#661

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[smoya]

cc @derberg @fmvilas as you both are code owners.

[jonaslagoni]

Is this not a breaking change? 🤔

[magicmatatjahu]

Is this not a breaking change? 🤔

If we switch optional to required then yes, but switching required -> optional won't break any AsyncAPI document.

[jonaslagoni]

If tooling expects channels to always be present, would that not break them?

[magicmatatjahu]

In can be "breaking change" in such a cases, but for example in the parser-js we handled from beginning case when channels are optional https://github.com/asyncapi/parser-js/blob/master/lib/models/asyncapi.js#L125 The question is should we look at tooling here?

[jonaslagoni]

In can be "breaking change" in such a cases, but for example in the parser-js we handled from beginning case when channels are optional https://github.com/asyncapi/parser-js/blob/master/lib/models/asyncapi.js#L125 The question is should we look at tooling here?

I would say so yes, especially since we are driven by tools. Just because our JS parser allows such a thing does not mean we should assume it won't be a problem elsewhere. However, kinda out of scope for this PR. Gonna create a new issue with a potential solution.

[magicmatatjahu]

I would say more that it's a breaking change on the tooling side and not the specification itself :)

[smoya]

I would say more that it's a breaking change on the tooling side and not the specification itself :)

Remember we do have hasChannels(): boolean; at Parser-JS document level, as some kind of protection.

[jonaslagoni]

Remember we do have hasChannels(): boolean; at Parser-JS document level, as some kind of protection.

While that is true, having support functions in a specific parser implementation does not remove the underlying problem 😄 One could ask why we even has such a function 😆 Added an issue: asyncapi/spec#688

[asyncapi-bot]

Hello, @jonaslagoni! 👋🏼

I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘

At the moment the following comments are supported in pull requests:

• /ready-to-merge or /rtm - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
• /do-not-merge or /dnm - This comment will block automerging even if all conditions are met and ready-to-merge label is added

[jonaslagoni]

@derberg your bot is acting weird! 😨

[derberg]

show some respect, bots that I train have feelings!

tbh, there is a bug (asyncapi/.github#121 (comment)) but this is super random, if the bug mentioned by me is really causing it, it should do it every time someone creates a comment, and not randomly 🤷🏼

anyway, thanks for 🏓 @jonaslagoni

[smoya]

While that is true, having support functions in a specific parser implementation does not remove the underlying problem 😄 One could ask why we even has such a function

I did a quick search at the very beginning of me creating this issue and didn't find any possible breaking change among our official tools. Did you find anything I missed?

[asyncapi-bot]

Hello, @smoya! 👋🏼

I'm Genie from the magic lamp. Looks like somebody needs a hand! 🆘

At the moment the following comments are supported in pull requests:

• /ready-to-merge or /rtm - This comment will trigger automerge of PR in case all required checks are green, approvals in place and do-not-merge label is not added
• /do-not-merge or /dnm - This comment will block automerging even if all conditions are met and ready-to-merge label is added

[fmvilas]

I did a quick search at the very beginning of me creating this issue and didn't find any possible breaking change among our official tools. Did you find anything I missed?

This would fail: https://github.com/asyncapi/asyncapi-react/blob/master/library/src/containers/Channels/Channels.tsx#L26

[smoya]

https://github.com/asyncapi/asyncapi-react/blob/master/library/src/containers/Channels/Channels.tsx#L26

What do you mean by "fail"? If you meant an error when executing, and as far as I understand, it won't enter the map function. I wrote a fiddle here to test it anyway https://jsfiddle.net/smoyac/9jhpnk6y/5/

I'm not saying we should or shouldn't consider it a breaking change. If our scope is all possible tools (not only those under AsyncAPI GH org), its fine.

[fmvilas]

What do you mean by "fail"?

What I mean is that Object.entries(undefined) triggers an error. One could say it's because we're not validating it but at the same time, the author of the tool can think it's safe to assume channels will not be undefined because it's been validated by the parser previously. If we make it optional then this breaks a tool. No matter how much we try to justify it, the user will perceive it as a breaking change because it's breaking their code.

[magicmatatjahu]

Regarding to this line, please have in mind that it would be checked in the main component if channels exist or not -> https://github.com/asyncapi/asyncapi-react/blob/master/library/src/containers/AsyncApi/AsyncApi.tsx#L123 So if someone won't write any channel it will work, even if someone will write channels: {}. In the next branch it is better handled, because we check length of keys (channels() function from parserJS always returns empty object if channels don't exist) -> https://github.com/asyncapi/asyncapi-react/blob/next/library/src/containers/Operations/Operations.tsx#L14

Of course other tools may have problems with this, but react component handles this problem :)

[fmvilas]

Yeah, our parser has a defense there. Still, we don't know what other parsers would do. AFAIK, there are parsers created by Mulesoft and WSO2. They could be easily assuming it's never going to be undefined because they validate it before but they don't transform undefined to {}.

[smoya]

By what we are talking here, Is it now clear we advocate for considering breaking changes any change that might break any tool around the world.
I'm super happy we are reaching that point. I agree this should be considered a breaking change, therefore to be included in version 3.0.0 of the spec.

I'd suggest we move our efforts to asyncapi/spec#688 and build a strong ruleset and guidelines so we don't need to worry about what is a breaking change or not anymore (likely).

[smoya]

/dnm

[derberg]

I agree better move to 3.0 and consider it a breaking change