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: switch model renders #53

Merged

Conversation

jonaslagoni
Copy link
Sponsor Member

@jonaslagoni jonaslagoni commented Feb 23, 2021

Description
This PR switches from QuickType model generation to our own implementation.

  • It removes the need for wrapping schemas inside messages and instead uses schemas directly.
  • Switches to a new hook.

Related issue(s)
Solves asyncapi/shape-up-process#50

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Very good! Only two comments :)

hooks/schema-generation.js Outdated Show resolved Hide resolved
hooks/schema-generation.js Outdated Show resolved Hide resolved
components/channel/OnReceivingData.js Outdated Show resolved Hide resolved
components/channel/OnReceivingData.js Outdated Show resolved Hide resolved
components/channel/OnReceivingData.js Outdated Show resolved Hide resolved
components/channel/OnReceivingData.js Outdated Show resolved Hide resolved
hooks/schema-generation.js Outdated Show resolved Hide resolved
@fmvilas
Copy link
Member

fmvilas commented Feb 26, 2021

I'm curious why you're generating the models on a hook instead of using these functions in a $$schema$$.js file. IMHO, it would be simpler to read.

hooks/schema-generation.js Outdated Show resolved Hide resolved
@magicmatatjahu
Copy link
Member

magicmatatjahu commented Feb 26, 2021

@fmvilas

I'm curious why you're generating the models on a hook instead of using these functions in a $$schema$$.js file. IMHO, it would be simpler to read.

asyncapi/generator#515 😅

Co-authored-by: Fran Méndez <fmvilas@gmail.com>
@fmvilas
Copy link
Member

fmvilas commented Feb 26, 2021

🤦 Thanks for pointing me to this, Maciej!

Co-authored-by: Fran Méndez <fmvilas@gmail.com>
@jonaslagoni
Copy link
Sponsor Member Author

I'm curious why you're generating the models on a hook instead of using these functions in a $$schema$$.js file. IMHO, it would be simpler to read.

@fmvilas I have no preferences, this is just how I did it with QuickType and to not mix different changes into one PR I will keep it like this. But it is a good point.

I am however unsure how I can even achieve this without the following issue to be addressed first: asyncapi/generator-react-sdk#10

Then I would have to iterate messages myself and generate models per payload 🤔

@jonaslagoni
Copy link
Sponsor Member Author

Why? I think you get the schema variable in the rendering context. It would be just a matter of calling the library with schema.json(). Or am I missing something?

Short answer yes you could do that. The problem is there are multiple ways of doing it depending on your use-case. If you used $$message$$ you could wrap all schemas for a specific message into a wrapper class, if you used $$objectSchema$$ (without this issue) you would probably generate multiple schemas depending on how you references them in your document with some duplicates that are never used.

However without this issue I don't think I would be able to incorporate it into a file schema the way I want it to😄

@fmvilas
Copy link
Member

fmvilas commented Feb 26, 2021

I don't completely follow but don't want to make you lose time. Let's discuss this next Wednesday on the retrospective 👍

@jonaslagoni
Copy link
Sponsor Member Author

@fmvilas created an issue to further investigate - #54

fmvilas
fmvilas previously approved these changes Mar 4, 2021
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

magicmatatjahu
magicmatatjahu previously approved these changes Mar 4, 2021
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM! You probably wait for asyncapi/generator-react-sdk#10, is it true?

@jonaslagoni
Copy link
Sponsor Member Author

@magicmatatjahu no I will solve the conflicts in a sec. #54 will be for waiting for that issue you linked :)

@jonaslagoni jonaslagoni dismissed stale reviews from magicmatatjahu and fmvilas via 786096c March 4, 2021 10:50
@jonaslagoni jonaslagoni merged commit 452f6e3 into asyncapi:master Mar 4, 2021
@jonaslagoni jonaslagoni deleted the feature/integrate_new_model_gen branch March 4, 2021 11:50
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 0.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@jonaslagoni
Copy link
Sponsor Member Author

@all-contributors please add @fmvilas review

@allcontributors
Copy link
Contributor

@jonaslagoni

I've put up a pull request to add @fmvilas! 🎉

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

4 participants