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

feat: add integration with AsyncAPI Server API #229

Merged
merged 23 commits into from Feb 17, 2022

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Jan 11, 2022

Description

  • add ServerAPI service

  • add GeneratorModal:

    image

    We can open it by clicking Generate template dropdown option:

    image

  • generate all available templates in our organization:

    • '@asyncapi/dotnet-nats-template'
    • '@asyncapi/go-watermill-template'
    • '@asyncapi/java-spring-cloud-stream-template'
    • '@asyncapi/java-spring-template'
    • '@asyncapi/java-template'
    • '@asyncapi/nodejs-template'
    • '@asyncapi/nodejs-ws-template'
    • '@asyncapi/python-paho-template'
    • '@asyncapi/ts-nats-template'
  • add script to generate JSON Schemas from template parameters (for each template) which we use to create form for passing parameters. In future it will be replaced by Define options for projects by JSON Schemas community#249 solution

Related issue(s)
Part of #86

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Jan 11, 2022
@netlify
Copy link

netlify bot commented Jan 11, 2022

✔️ Deploy Preview for modest-rosalind-098b67 ready!

🔨 Explore the source changes: e659201

🔍 Inspect the deploy log: https://app.netlify.com/sites/modest-rosalind-098b67/deploys/620a852424fc2a0008748cf4

😎 Browse the preview: https://deploy-preview-229--modest-rosalind-098b67.netlify.app

@jonaslagoni
Copy link
Sponsor Member

Just a quick comment, I don't feel the generate template reflects what it does 🤔 You don't generate a template, you generate with a template.

However, you cant say generate code as that's not entirely accurate 😅 generate stuff, generate code/docs hmm, @alequetzalli maybe you have a suggestion for better wording 🤔 ?

@alequetzalli
Copy link
Member

Just a quick comment, I don't feel the generate template reflects what it does 🤔 You don't generate a template, you generate with a template.

However, you cant say generate code as that's not entirely accurate 😅 generate stuff, generate code/docs hmm, @alequetzalli maybe you have a suggestion for better wording 🤔 ?

I see, in that case @jonaslagoni and @magicmatatjahu, how about the following idea?

Where is currently reads: Generate template based on AsyncAPI Document, how about changing it to Generate code/docs with a template.

Then underneath where it simply says Template, how about adding description text to it that reads Your template will be based on your AsyncAPI Document.

Screen Shot 2022-01-21 at 2 32 17 PM

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Jan 24, 2022

@alequetzalli

Where is currently reads: Generate template based on AsyncAPI Document, how about changing it to Generate code/docs with a template.

Then underneath where it simply says Template, how about adding description text to it that reads Your template will be based on your AsyncAPI Document.

Great! I'll add that :) However, what about node in dropdown? Generate template, or Generate docs/code as @jonaslagoni suggested? WDYT?

image

EDIT:

Updated :)

image

@alequetzalli
Copy link
Member

alequetzalli commented Jan 26, 2022

Generate docs/code

@magicmatatjahu Ah! For that drop down, I would do what @jonaslagoni suggested too and replace it with Generate docs/code.

Screen Shot 2022-01-25 at 8 42 01 PM

@boyney123
Copy link
Contributor

@magicmatatjahu looking good mate!

I have a few questions, ideas and feedback around it, wondered what you thought?


Template Selection Naming

It currently renders the package name (from what I can tell), I wonder if there is a more user-friendly way of showing this, without having to understand the package names etc?

Here is what it renders now 👇
image

Some Ideas 👇 (not all there, but some)

image

Think this might give more context for the user what they are doing?


Hide params that are not required (with the option to show)

When you select a template the user is shown the params for that template, I'm assuming some of these are optional ones? Is it possible to have only the required ones shown and "Advanced Options" for the optional ones. I think this could simplify the UX for people? Just a thought!

image


Link to Package Docs when clicking a Template

Coming at some of these templates new, it's hard to understand what the options mean even tho the description is there. I wonder if a link to the GitHub repo/docs for the templates could help people gain details if they need it?


These are just some thoughts after the initial look, wonder what you all think?

@magicmatatjahu
Copy link
Member Author

@boyney123 Thanks very much for that feedback! I propagated your ideas to code. Could you check? :)

alequetzalli
alequetzalli previously approved these changes Feb 2, 2022
Copy link
Member

@alequetzalli alequetzalli left a comment

Choose a reason for hiding this comment

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

🚢🚢🚢

@boyney123
Copy link
Contributor

Nice one @magicmatatjahu !

Dropdown is great with the additional context!

Not sure if it's just me but the Advanced Options placement of the button feels off to me? wonder if there is somewhere else we could put it?

image

Maybe something like this?

image

What do you think @magicmatatjahu? Just an idea 🤔

@magicmatatjahu
Copy link
Member Author

@boyney123 Ok I propagated your suggestions :) Could you check again?

@sonarcloud
Copy link

sonarcloud bot commented Feb 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@magicmatatjahu
Copy link
Member Author

@mcturco @boyney123 Could you check final (I hope) implementation? 😄

@mcturco
Copy link
Member

mcturco commented Feb 14, 2022

@magicmatatjahu Hi 👋

It could just be me & my lack of spec knowledge, but the Advanced options in the modal for the HTML website seem a bit vague (I haven't looked through the options for the other outputs)

Screen Shot 2022-02-14 at 1 55 02 PM

I am wondering if we should add placeholder text to the inputs so there is an example of what parameter to use.

sidebarOrganization to me should be a set of two radio buttons (or checkboxes, depending on whether you could enable both options at the same time or not), since there are only 2 options. I foresee a lot of user error with this since it is a text input with only 2 strings you can add in there.

Maybe in baseHref there should be some placeholder text like /example-url/ so the user is aware of what syntax to use.

version - is this option really necessary? Couldn't they just adjust the version in the studio editor before they click Generate code/docs? I am just wondering how many use cases this option offers.

singleFile - the verbiage in the description text here is confusing. Perhaps it should say Enable this option to generate a single HTML file with scripts and styles added within

outFileName - I think we could add some placeholder text here that says example.html and wondering if the .html part could be added automatically in the visual logic of the input? So the user should be able to just type the file name without having to enter .html but they will still visually see the file extension within the input

pdf - Tbh I am not really a fan of the options being presented in their schema name, and it is specifically bothering me for this parameter. IMO it should say Generate PDF instead of just pdf. Other than that, I think we can update the verbiage to match the other true/false toggle that I commented on above: Enable this option to include a PDF version of your documentation website


Just took a look at some of the other advanced options for different outputs... WOW there are a lot of options under Java Spring Project. Are these all necessary options? Are there any options that can get combined or show/hide based on server configuration? I am seeing Only for Kafka in a couple of places and Only for MQTT in some other places.... could we possibly only show these options if the server in the AsyncAPI file is set to that specific server?


Last note: I am seeing this as an "error" message in one of the output options. AsyncAPI document doesn't have at least one server with supported protocols. For the selected generation, these are supported: ws

Screen Shot 2022-02-14 at 2 09 33 PM

Shouldn't we properly spell out the protocol so that the user knows what to correct in their AsyncAPI file to make this error disappear? ws --> WebSocket OR maybe we should be more human about it 😄 We could say: Configure your AsyncAPI document server protocol to WebSocket to generate a NodeJS WebSocket Project

@magicmatatjahu
Copy link
Member Author

@mcturco Thanks for that awesome feedback! I'm a little remorseful that you wrote so much and the way it looks is due to a certain problem we're currently having in the https://github.com/asyncapi/generator side. I will describe it on the HTML Generation example.

Every template (project that the Generator can generate) has a some set of available "parameters". For html there you have list of them https://github.com/asyncapi/html-template/blob/master/package.json#L78 as you can see you have only description, default value and information if given parameter is required or not (required: true | false). You can also check another templates, like for Java where you have a lot parameters. I don't want to "render" manually that parameters because it would be insane to "manually" write description, check available option (to render <select> HTML tag) etc. so I base the current implementation on available data of each parameter: key (name of parameter), description, required, default. That's why some parameters (or rather their description) look like this. I agree that some parameters should be in the form of options (select), not inputs, but it will not be a good solution to do it manually or infer the description of the parameter itself. We have issue to add more option for defining generator's parameter -> asyncapi/generator#414 You can read whole issue (if you have time 😆 ) because it's very interesting.

Of course I agree with all the objections about the descriptions and that they should be better described, but this needs to be changed in the html-template project itself (here https://github.com/asyncapi/html-template/blob/master/package.json#L78), not in this PR, because here we have script which retrieve all parameters data which we can consume on the UI side to render that forms.

I am wondering if we should add placeholder text to the inputs so there is an example of what parameter to use.

We cannot make any placeholder because we don't know how data we can put in it 😞


Just took a look at some of the other advanced options for different outputs... WOW there are a lot of options under Java Spring Project. Are these all necessary options? Are there any options that can get combined or show/hide based on server configuration? I am seeing Only for Kafka in a couple of places and Only for MQTT in some other places.... could we possibly only show these options if the server in the AsyncAPI file is set to that specific server?

Look above :) We need to support that asyncapi/generator#414 to achieve that.


Last note: I am seeing this as an "error" message in one of the output options. AsyncAPI document doesn't have at least one server with supported protocols. For the selected generation, these are supported: ws
Shouldn't we properly spell out the protocol so that the user knows what to correct in their AsyncAPI file to make this error disappear? ws --> WebSocket OR maybe we should be more human about it

Yeah we can make some mapping like ws -> WebSocket, http -> HTTP :) No problem, thanks for idea! However:

We could say: Configure your AsyncAPI document server protocol to WebSocket to generate a NodeJS WebSocket Project

you may have several protocols that are required for a given template (like for java https://github.com/asyncapi/java-spring-template/blob/master/package.json#L74) that's why I think it's easier to display the list of supported protocols at the end of the sentence instead of in the middle :)


Thanks again for that feedback and sorry you had to write so much! I hope that at least from my comment you understood how the parameters in the generator work :)

@mcturco
Copy link
Member

mcturco commented Feb 14, 2022

the way it looks is due to a certain problem we're currently having in the https://github.com/asyncapi/generator side.

@magicmatatjahu gotcha. I understand now that the Generator is the source for the options to render output, thank you for explaining that! Hmm I still think that we should consider the notes I left even if we have to do it in a separate PR for the generator. I just feel like the way the UX is now in the studio makes it hard for the user to know what to do to implement the advanced options, but we can of course do a usability test to see if thats just my opinion or if people will actually have a problem with how it is now.

Sorry if I misunderstood what I was supposed to be reviewing 😅

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Feb 14, 2022

@mcturco

No worries :) Good that you did this review because it shows that actually this UX is not working and it needs to be improved on the generator side to render the form better. However, I would like you to do a review if the UX itself (not taking into account those problems we have with parameters) is ok, so thank you very much! :) I will try tomorrow to implement this mapping ws -> WebSocket etc.

@mcturco
Copy link
Member

mcturco commented Feb 15, 2022

@magicmatatjahu sure, no problem. But yeah I think it definitely is cool to see the generator working within Studio 😄 I think the option to download code is relatively easy to use (if we don't think about the potential issues that I brought up, haha)

I say we should ship! 🚢

@magicmatatjahu
Copy link
Member Author

/rtm

@asyncapi-bot asyncapi-bot merged commit b6fcc2b into asyncapi:master Feb 17, 2022
@magicmatatjahu magicmatatjahu deleted the server-api-integra branch February 17, 2022 16:27
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

None yet

6 participants