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: add convert functionality #100

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Oct 1, 2021

Description

  • Add convert functionality - add new node in the editor dropdown - convert
  • Add functionality to load AsyncAPI doc from base64 - new node in the editor dropdown
  • Add relevant unit tests

image

Related issue(s)
See also #80

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Oct 1, 2021
@magicmatatjahu magicmatatjahu marked this pull request as ready for review October 4, 2021 22:07
@magicmatatjahu
Copy link
Member Author

@boyney123 Do you wanna make small review? :) I cannot assign you to reviewers because you don't yet in organization :( Talk about this with Łukasz.

@boyney123
Copy link
Contributor

Cool yeah will do @magicmatatjahu

Copy link
Member

@jonaslagoni jonaslagoni left a comment

Choose a reason for hiding this comment

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

I have no comments for this PR 👍

On a side-note, is it possible we can integrate preview workflow, as the website has, for PR's, so we don't have to download and run in manually?

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Oct 5, 2021

@jonaslagoni Thanks! As far as I know Netlify even on the Open Source plan has the ability to configure previews for repositories in the same organization, but I would have to have admin privileges to do that I will talk to Łukasz.

@boyney123
Copy link
Contributor

Cool, just got it running locally and read the code. Most of it looked fine to me, (I would have to get my head around hookstate and the services pattern used, but no a problem).

There were a few things I noticed when using it:

See which version you are currently on

Convert AsyncAPI document

Very minor thing, and you would hope the user knows what version they are currently on, but maybe in this modal we could show that the user is on version x going to be bumped to version y.

Again minor here, but maybe the versions should go from high to low?

image

Example

  • Latest
  • 2.2.0 (prob not required if its the latest)
  • 2.1.0
  • 2.x.x (you get the idea)

No way to go between versions

I guess maybe why would you want to do this, but if I convert my spec, there is no way to convert it back, but I guess this is out of scope and maybe not even required??.....

Import base64

I entered random values and got a success message

image

And my document is gone

image

Maybe we could valididate the value somehow and allow do the processing if its valid?

Tests

Tests look cool for the services, just wondering how the project goes or will go about testing components (or if you intend too?)

Cheers!

@magicmatatjahu
Copy link
Member Author

magicmatatjahu commented Oct 5, 2021

@boyney123 Thanks for review!

Very minor thing, and you would hope the user knows what version they are currently on, but maybe in this modal we could show that the user is on version x going to be bumped to version y.

I can render currently version in the title :) I will show you my idea in few minutes.

Again minor here, but maybe the versions should go from high to low?

Good idea 👍🏼 I will propagate it.

I guess maybe why would you want to do this, but if I convert my spec, there is no way to convert it back, but I guess this is out of scope and maybe not even required??.....

It's out of scope task and we don't support such a thing in our converter. I don't see any advantages to convert to older version - in 2.2.0 we have only addition, so with decreasing the version, we should remove new things in 2.2.0 - If you really see some use cases, please create issue to starting discussion, but not in thsi repo, but rather in converter-js :)

Maybe we could valididate the value somehow and allow do the processing if its valid?

I know about this problem, but it works in "similar" way like normal IDE and our parser. You can pass some invalid AsyncAPI doc and you will have appropriate error (you can see in terminal when you click block under editor), that asyncapi field is missed - it's a valid AsyncAPI doc. In addition to base64, downloading from urls and from a file works identically. The parsing is done anyway when the new content is added/passed to the editor, so user should know exactly what are downloading/adding. Of course, maybe my thinking is wrong, and we try to "parse" spec before changing content in editor, but not in this PR 😄 I am opened to ideas how to achieve that.

Tests look cool for the services, just wondering how the project goes or will go about testing components (or if you intend too?)

Tests for components will be, but as you can see the components themselves use services underneath that have business logic implemented - using state, effects is as little as possible etc. This is something I hate in front-end, because testing components is always 90% mocking.

If we go in the direction of modules - asyncapi/asyncapi-react#433 - then the services themselves will be connected to the IOC container and they will be created dynamically in the runtime, not like now that everything is a static singleton and testing of components will be simplified. Services in the components will then look like:

const Component: () => {
  const specService = useInject(SpecificationService);
  
  ...
}

At the moment there is no point in waiting with the modules implementation because the Studio would be implemented next year 😆 and it is better to release something and then improve it. Switching to modules should take one day, or even less, and the static services themselves do not currently cause many problems :)

@magicmatatjahu
Copy link
Member Author

@boyney123 My proposition :)

image

@boyney123
Copy link
Contributor

Thanks @magicmatatjahu

this is something I hate in front-end, because testing components is always 90% mocking.

haha yeah tell me about it 😅

At the moment there is no point in waiting with the modules implementation because the Studio would be implemented next year...

Ah yeah, no problem I will read about the modules sounds interesting for sure!

It's out of scope task and we don't support such a thing in our converter. I don't see any advantages to convert to older version - in 2.2.0 we have only addition, so with decreasing the version, we should remove new things in 2.2.0 - If you really see some use cases, please create issue to starting discussion, but not in thsi repo, but rather in converter-js :)

Yeah it's a bit weird, for some reason I just expected to be able to do it but think that was just YOLO time, me clicking around. If I get any use case will raise it for sure!

@magicmatatjahu
Copy link
Member Author

@boyney123 So I guess that everything is good, or you wanna add something more?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 5, 2021

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
No Duplication information No Duplication information

@boyney123
Copy link
Contributor

Let's go @magicmatatjahu 🚀

@magicmatatjahu magicmatatjahu merged commit 3d8a8f2 into asyncapi:spa-scratch Oct 5, 2021
@magicmatatjahu magicmatatjahu deleted the spa-scratch/editor-sidebar-convert-base64 branch October 5, 2021 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants