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

fix: add interface Options, remove pre-validation with Parser #188

Merged

Conversation

aeworxet
Copy link
Collaborator

This PR:

  • switches Bundler from using custom types to using types from @asyncapi/parser;
  • introduces interface Options to improve DX when working with Bundler's options.

Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

Initially we decided that bundler would not depend on asyncapi parser #26 can you discuss why benifit we are getting from using for asyncapi-parser.

package.json Outdated Show resolved Hide resolved
@aeworxet
Copy link
Collaborator Author

It appeared that Bundler doesn't always receive a valid AsyncAPI Document as an input. A non-valid AsyncAPI Document, which still remains to be a valid JSON/YAML, passes the bundling process because @apidevtools/json-schema-ref-parser doesn't care if a valid JSON is also a valid AsyncAPI Document. If there was an error in the initial AsyncAPI Document, it goes further, and as a result, the user ends up wondering what's wrong with their AsyncAPI Document if no errors were reported in between.

Users also don't always validate the outputted AsyncAPI Document because they used The Official App for bundling; how could it have bundled something improperly?

So, practice has shown that Bundler ought to satisfy the user's hope of getting a fully valid AsyncAPI Document without the need to use any other tool.

In fact, I would prefer to validate the data twice, both input and output, but validating the input wouldn't catch errors in the $refed AsyncAPI Documents. So, I want to at least validate the output, which will also catch the wrong input, if there was any.

And if I use the Parser anyway, then I can also delegate it watching over new formats (Spec versions) of AsyncAPI Documents, eliminating the need to update local types with each major change in the Spec.

@Souvikns
Copy link
Member

If the user needs to validate the specifications, then the user can validate the spec using @asyncapi/parser before the bundling process. We can do this in CLI as well so users using the CLI will always get validated before bundle process starts. Anyone installing the bundler wants to use it as a library and so they can custom validate thier spec.

@aeworxet
Copy link
Collaborator Author

@derberg
Should the approach described in #26 be reconsidered since two years have passed already, or should I remove the pre-validation with Parser and the @asyncapi/parser dependency altogether (pre-validation has proven useful by catching errors early, and also in Optimizer (I'll have to remove it from there too)?)

@derberg
Copy link
Member

derberg commented Aug 29, 2024

yeah, basically if parser is just used for validation and nothing else, can be removed and just readme should be clear about it, and that if someone needs validation, they should use parser. And as @Souvikns wrote, in CLI we can use existing code to enable validation before bundling

@aeworxet aeworxet changed the title fix: use types from @asyncapi/parser and interface Options fix: add interface Options Sep 2, 2024
@aeworxet
Copy link
Collaborator Author

aeworxet commented Sep 2, 2024

Pre-validation with Parser helped me catch a lot of early errors, but if you both insist. 🤷

It was never documented; it simply was there for several versions, so I didn't change the README.md.

@aeworxet aeworxet changed the title fix: add interface Options fix: add interface Options, remove pre-validation with Parser Sep 2, 2024
@aeworxet aeworxet changed the title fix: add interface Options, remove pre-validation with Parser fix: add interface Options, remove pre-validation with Parser Sep 2, 2024
@aeworxet aeworxet force-pushed the fix-improve-working-with-types branch from 604c268 to 054b1e9 Compare September 2, 2024 04:21
Copy link

sonarcloud bot commented Sep 2, 2024

Copy link
Member

@Souvikns Souvikns left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member

derberg commented Sep 3, 2024

prevalidation with parser is still recommended, it's just that it should be documented and suggested in readme. If someone wants to use bundler on their own, they should also use parser-js and in case of AsyncAPI CLI, bundling should be done with initial validation using parser that is there in CLI already

@aeworxet
Copy link
Collaborator Author

aeworxet commented Sep 5, 2024

I'll address that in a separate README update then.

@aeworxet
Copy link
Collaborator Author

aeworxet commented Sep 5, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit f4e1224 into asyncapi:master Sep 5, 2024
10 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.6.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aeworxet aeworxet deleted the fix-improve-working-with-types branch September 5, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants