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

React templates make generation process longer #521

Closed
derberg opened this issue Mar 1, 2021 · 33 comments · Fixed by #1177
Closed

React templates make generation process longer #521

derberg opened this issue Mar 1, 2021 · 33 comments · Fixed by #1177

Comments

@derberg
Copy link
Member

derberg commented Mar 1, 2021

Describe the bug

    ✓ generated using Nunjucks template (492ms) #html-template
    ✓ generate using React template (4875ms) #markdown-template

Markdown template is the simples one we have, yet it takes 5s to generate asyncapi.md with it. Which is really a lot and I bet it will be worse with more complicated templates.

How to Reproduce

Just try to generate with HTML template and then markdown one and you'll see that there is a time difference each time.

The problem is in transpilation process with babel. @magicmatatjahu checked and:

5 times in a row with transpilation in each render:
3.768s
2.391s
2.236s
2.260s
2.414s
Average: ~2.6s
And here is a time without transpilation in each render:
191.515ms
194.49ms
185.167ms
192.192ms
199.48ms
Average: ~0.193s

Expected behaviour

IMHO by default, when you run generation, an already transpiled template is used to speed up (yes, it means we publish transpiled files to npm too). The generator is smart and spots transpiled files are missing and it needs to run transpilation. Users (template developers) also have a flag that allows them to explicitly run transpilation during development

@magicmatatjahu
Copy link
Member

magicmatatjahu commented Mar 1, 2021

I see two options to have transpiled template in npm:

  • --compile flag in our cli - it probably the easiest way to resolve problem and it provides a good DX for template's developer, but I don't know if new flag should work only with React.
  • instruct people how to write simple script in js to run transpilation in prepublish script in package.json - suggested by @jonaslagoni

@derberg
Copy link
Member Author

derberg commented Mar 1, 2021

--compile flag in our cli - it probably the easiest way to resolve problem and it provides a good DX for template's developer, but I don't know if new flag should work only with React.

I think it is ok that --compile is just for react as anyway this is a flag that is related purely to development only

instruct people how to write simple script in js to run transpilation in prepublish script in package.json.

I think this is fine, we just need to make it clear in docs and present it in the template for templates

@magicmatatjahu
Copy link
Member

Let's wait for other answers, what they think about it. After that, I would like to implement the solution.

@magicmatatjahu
Copy link
Member

@derberg Maybe we should transfer this issue to generator repo? I mean, in this repo shouldn't be changed anything, issue is only related with generator.

@derberg derberg transferred this issue from asyncapi/generator-react-sdk Mar 2, 2021
@derberg
Copy link
Member Author

derberg commented Mar 2, 2021

@magicmatatjahu good point, done

@github-actions
Copy link
Contributor

github-actions bot commented May 2, 2021

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label May 2, 2021
@derberg derberg removed the stale label May 4, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2021

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label Jul 4, 2021
@derberg derberg removed the stale label Jul 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 4, 2021

This issue has been automatically marked as stale because it has not had recent activity 😴
It will be closed in 60 days if no further activity occurs. To unstale this issue, add a comment with detailed explanation.
Thank you for your contributions ❤️

@github-actions github-actions bot added the stale label Sep 4, 2021
@derberg derberg removed the stale label Sep 6, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Jan 5, 2022

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jan 5, 2022
@derberg
Copy link
Member Author

derberg commented Jan 11, 2022

@magicmatatjahu I don't remember what compile flag was suppose to do

@github-actions github-actions bot removed the stale label Jan 12, 2022
@magicmatatjahu
Copy link
Member

@derberg To compile the React template and use that "compiled" templated from each run. At the moment we transpile/compile React component by babel on each generation and it is a waste of time.

@derberg
Copy link
Member Author

derberg commented Jan 12, 2022

@magicmatatjahu thanks, now I remember. I just think now that it should work like this by default. When you run generation, generator first look for sources of compiled template, if provided, and if not, performs old way. And yeah still in template for templates we need to show how to get it into npm

@magicmatatjahu
Copy link
Member

I just think now that it should work like this by default. When you run generation, generator first look for sources of compiled template, if provided, and if not, performs old way

Exactly in this way, look for transpiled files, if don't exist then run transpilation/compilation and generate template.

@jonaslagoni
Copy link
Member

jonaslagoni commented Feb 14, 2022

I am getting a bit tired of the long transpile process 😅

@magicmatatjahu would you be able to formulate this into a good first issue? 🙂

@derberg
Copy link
Member Author

derberg commented Mar 29, 2022

@magicmatatjahu I second what @jonaslagoni wrote because of this thread https://asyncapi.slack.com/archives/CQVJXFNQL/p1648304288930959

we are getting more and more React-based templates, means more unhappy users.

btw, I was wondering if we could not make it

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Jul 28, 2022
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Nov 26, 2022
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 26, 2023
@derberg derberg removed the stale label Dec 18, 2023
@derberg
Copy link
Member Author

derberg commented Dec 18, 2023

Reopening. Now with budget in bounty program maybe we can find someone who could work on improving things

@derberg derberg reopened this Dec 18, 2023
@derberg
Copy link
Member Author

derberg commented Dec 18, 2023

What needs to be done:

  1. In Generator we need to:

First bullet point might be considered a breaking change, which is not a problem as we can release it as breaking together with #925 and also to use this feature, template developers will anyway have to make changes in their projects to enable release of transpiled files - so they can also change template settings so new version of their template supports latest generator

  1. In CLI new flag will need to be enabled

  2. In html-template we need an example flow on how to enable this new feature to speed up development but also DX in case of generation of output

  3. Educate template developers: have a document that describes how stuff work and make sure in all templates under AsyncAPI org there are issues to set things up

1-3 can be pushed through bounty program
2-4 can be pushed as good first issues as after 1 and 3, 2 and 4 will be trivial

@jonaslagoni @magicmatatjahu sounds like a plan? also we could use this as onboarding of potential new maintainer in generator

@jonaslagoni
Copy link
Member

@derberg makes sense to me 👌

@smoya
Copy link
Member

smoya commented Dec 18, 2023

A non-asked opinion here (not owner): I like the suggested solution 👍

@derberg
Copy link
Member Author

derberg commented Mar 14, 2024

actually I think number 1 is not needed

I opened a bounty issue for number 3 asyncapi/html-template#558. I noticed that only first generation takes time, and then once transpilation files are there, it is fast, so looks like babel that we use underneath is pretty smart. So lets first modify release for html-template and see.

@Gmin2
Copy link
Collaborator

Gmin2 commented Mar 31, 2024

Hey @derberg as asyncapi/html-template#558 is near to completion can i open number 3 issue in the CLI

@derberg
Copy link
Member Author

derberg commented Apr 2, 2024

Ok, so asyncapi/html-template#558 is completed and __transpiled folder is now part of package, but yeah, we still need to do modifications in generator as logs still show:

[BABEL] Note: The code generator has deoptimised the styling of /xxx/.nvm/versions/node/v16.13.0/lib/node_modules/@asyncapi/cli/node_modules/@asyncapi/generator/node_modules/@asyncapi/html-template/template/js/asyncapi-ui.min.js as it exceeds the max of 500KB.

which means transpilation runs locally anyway

of course it runs faster as some files are there, as they are now included in a package - but we should anyway try to avoid triggering transpilation if not needed

//these are times for template v2.2 without transpiled files

real	1m17.492s
user	0m37.739s
sys	0m24.042s

//these are times after transpilation is included in the package in v2.3
real	0m19.055s
user	0m25.391s
sys	0m3.009s

so we gained at speed significantly anyway! thanks @utnim2

@derberg
Copy link
Member Author

derberg commented Apr 2, 2024

is near to completion can i open number 3 issue in the CLI

@utnim2 CLI is point 2 and we need point 1 first. Should be an easy PR 🤔 you think you can work on it?

@Gmin2
Copy link
Collaborator

Gmin2 commented Apr 2, 2024

@utnim2 CLI is point 2 and we need point 1 first. Should be an easy PR 🤔 you think you can work on it?

is point 1 needed you have said here that point 1 is not needed

@derberg
Copy link
Member Author

derberg commented Apr 2, 2024

yeah, so we see optimization worked but mainly because of babel that is smart and just tries to regenerate what is not available. But logs still show babel is triggered, so yeah, we should skip it entirely if not needed. So generator should perform transpilation only if explicitly "asked" or if transpiled folder is not present

@Gmin2
Copy link
Collaborator

Gmin2 commented Apr 2, 2024

@derberg will you make the issue or should I proceed with a PR directly

@derberg
Copy link
Member Author

derberg commented Apr 3, 2024

@utnim2 no need for separate issue, just refer to this one

@Gmin2
Copy link
Collaborator

Gmin2 commented Apr 4, 2024

@utnim2 CLI is point 2 and we need point 1 first. Should be an easy PR 🤔 you think you can work on it?

Done in this #1177
cc @derberg

Copy link
Contributor

github-actions bot commented Aug 3, 2024

This issue has been automatically marked as stale because it has not had recent activity 😴

It will be closed in 120 days if no further activity occurs. To unstale this issue, add a comment with a detailed explanation.

There can be many reasons why some specific issue has no activity. The most probable cause is lack of time, not lack of interest. AsyncAPI Initiative is a Linux Foundation project not owned by a single for-profit company. It is a community-driven initiative ruled under open governance model.

Let us figure out together how to push this issue forward. Connect with us through one of many communication channels we established here.

Thank you for your patience ❤️

@github-actions github-actions bot added the stale label Aug 3, 2024
@derberg
Copy link
Member Author

derberg commented Aug 19, 2024

still in progress - almost there

@github-actions github-actions bot removed the stale label Aug 20, 2024
@derberg
Copy link
Member Author

derberg commented Aug 26, 2024

this issue will be closed with #1177

some followup will be done with #1259

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

Successfully merging a pull request may close this issue.

5 participants