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: render react app setup into dedicated js file #686

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

mwiesenbauer
Copy link
Contributor

@mwiesenbauer mwiesenbauer commented Oct 2, 2024

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.

singleFile option is still supported with this change

Copy link

@github-actions github-actions bot left a 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.

@mwiesenbauer mwiesenbauer changed the title render react app setup into dedicated js file feat: render react app setup into dedicated js file Oct 2, 2024
@derberg
Copy link
Member

derberg commented Oct 14, 2024

lgtm, but please make sure we still support an option that all files can be also inlined in one file with singleFile property

@mwiesenbauer mwiesenbauer force-pushed the master branch 2 times, most recently from f038b9d to f79887c Compare October 14, 2024 09:04
Copy link
Member

@derberg derberg left a 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?

@mwiesenbauer
Copy link
Contributor Author

Thanks for testing!

I understand your reasoning and do not have any strong preferences either way.
I would probably go with a minor release, as a major version seems overkill for me.

Copy link
Member

@derberg derberg left a 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.

@mwiesenbauer
Copy link
Contributor Author

@derberg Sorry for the delay, done.

Copy link

@derberg
Copy link
Member

derberg commented Nov 4, 2024

thanks a lot for the PR and patience with getting it merged.

I got no response from community on my major/minor concerns, so this time we go with minor 🤷🏼

/rtm

@asyncapi-bot asyncapi-bot merged commit 8a93359 into asyncapi:master Nov 4, 2024
13 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.1.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants