-
-
Notifications
You must be signed in to change notification settings - Fork 59
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: render react app setup into dedicated js file #686
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
lgtm, but please make sure we still support an option that all files can be also inlined in one file with |
f038b9d
to
f79887c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the work.
I also tested manually:
### default
asyncapi generate fromTemplate ./test/docs/dummy.yml https://github.com/mwiesenbauer/html-template -o ./dupa --force-write --use-new-generator
and
### single file
asyncapi generate fromTemplate ./test/docs/dummy.yml https://github.com/mwiesenbauer/html-template -o ./dupa --force-write --use-new-generator -p singleFile=true
I have only once concern - if this is not a breaking change.
in theory, there is no change in functionality, but imho output is part of template "functionality", especially in case of code/docs generators. This new html-template version will output by default one extra file - MUST HAVE file. This may affect pipelines if people configure them explicitly to react only on certain output files by their name, you know? like copy not entire directory output but just certain files, for various reasons - maybe security.
so I'm thinking if we should not do major here
any thoughts?
Thanks for testing! I understand your reasoning and do not have any strong preferences either way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, forgot you need to update tests. Notice they are failing. These are snapshot test. Problem is that in tests we test directly Index
function but it's output is now different, so you will need to update the snapshot test to reflect this change, and push to that PR for review. When running tests locally, just add manually --updateSnapshot
flag to script where jest is invoked (you can also add new script for it and push to this PR. Also you need to extend the snapshot test by also adding a code verification of the new App
component rendering.
@derberg Sorry for the delay, done. |
Quality Gate passedIssues Measures |
The HTML Template currently renders inline javascript with schema and react application setup code.
Inline javascript in combination with a content security policy (CSP) is discouraged.
Rendering schema and application setup code into a dedicated javascript file will resolve this issue.