[GH-ISSUE #1544] Refactoring Suggestion - Codegen generators #491

Closed
opened 2026-03-16 15:39:08 +03:00 by kerem · 10 comments
Owner

Originally created by @JayceDugan on GitHub (Mar 17, 2021).
Original GitHub issue: https://github.com/hoppscotch/hoppscotch/issues/1544

Originally assigned to: @JayceDugan on GitHub.

Overview

After reviewing the existing implementation of codegen generators present in /helpers/codegen/generators/*.js, I'd like to gather thoughts on refactoring the existing implementations to provide the following benefits:

  • Improved readability.
  • Reduced complexity.
  • Simpler and more expressive architecture.
  • Improve separation of concerns.
  • Improved extensibility / re-use depending on implementation through class inheritance if opportunities present.

Please find considerations and questions at the bottom of this description.

Note:

Once this issue is created I will be linking a diff comparison in the comments below of a loose example suggestion of what a refactoring might look like.

Current File Implementation
Example refactoring file
Example refactoring PR Diff

IMPORTANT - This is not to be considered a proposed solution of exactly how the refactoring would be implemented across all generators, and is more to be considered a loosely proposed idea of what a refactoring might look like of an existing implementation as a sample case.

More discussion is needed first before an exact architecture solution is proposed and additional review should be done across all generators to find common properties and opportunities for an inheritance structure, not to mention any ideas that come as a result of this discussion not previously thought of.

Proposed steps:

  1. Review existing test spec implementations for individual generators before refactoring or changing any existing structure to confirm no modifications have been made to existing functionality.
  2. Write test specs for individual generators if applicable.
  3. Extract actions in existing generator() methods to isolated functions for separation of concerns.
  4. Extract conditional logic in new isolated functions to improve separation of concerns.
  5. Review isolated methods and refactor if required for improved readability of method purpose & naming conventions.
  6. Refactor isolated methods to class structure, and potentially consider a parent Generator class for common properties ( requestAllowsBody(), isBasicAuthenticationRequest(), isTokenAuthenticationRequest() etc if applicable... )

Alternatives

Simply completing steps 1, 2 and 3 would improve the existing implementations for future maintenance, improved readability and reduced complexity.

Considerations & Questions

  • Do we have existing tests surrounding the existing implementations?
  • If applicable, what is the test coverage like for the existing implementations?
  • If no existing test coverage is present, what test coverage should be present before refactoring is implemented?
Originally created by @JayceDugan on GitHub (Mar 17, 2021). Original GitHub issue: https://github.com/hoppscotch/hoppscotch/issues/1544 Originally assigned to: @JayceDugan on GitHub. ## Overview After reviewing the existing implementation of codegen generators present in `/helpers/codegen/generators/*.js`, I'd like to gather thoughts on refactoring the existing implementations to provide the following benefits: - Improved readability. - Reduced complexity. - Simpler and more expressive architecture. - Improve separation of concerns. - Improved extensibility / re-use depending on implementation through class inheritance if opportunities present. Please find considerations and questions at the bottom of this description. ### Note: Once this issue is created I will be linking a diff comparison in the comments below of a loose example suggestion of what a refactoring **might** look like. [Current File Implementation](https://github.com/JDevx97/hoppscotch/blob/main/helpers/codegen/generators/salesforce-apex.js) [Example refactoring file](https://github.com/JDevx97/hoppscotch/blob/0effd2cce9a8466ad76c92c46837666dba7878e3/helpers/codegen/generators/salesforce-apex.js) [Example refactoring PR Diff](https://github.com/JDevx97/hoppscotch/pull/1/files) **IMPORTANT** - This is not to be considered a proposed solution of exactly how the refactoring would be implemented across all generators, and is more to be considered a loosely proposed idea of what a refactoring *might* look like of an existing implementation as a sample case. More discussion is needed first before an exact architecture solution is proposed and additional review should be done across all generators to find common properties and opportunities for an inheritance structure, not to mention any ideas that come as a result of this discussion not previously thought of. ## Proposed steps: 1. Review existing test spec implementations for individual generators before refactoring or changing any existing structure to confirm no modifications have been made to existing functionality. 2. Write test specs for individual generators if applicable. 3. Extract actions in existing `generator()` methods to isolated functions for separation of concerns. 4. Extract conditional logic in new isolated functions to improve separation of concerns. 5. Review isolated methods and refactor if required for improved readability of method purpose & naming conventions. 6. Refactor isolated methods to class structure, and potentially consider a parent `Generator` class for common properties ( `requestAllowsBody(), isBasicAuthenticationRequest(), isTokenAuthenticationRequest() etc if applicable... `) ## Alternatives Simply completing steps 1, 2 and 3 would improve the existing implementations for future maintenance, improved readability and reduced complexity. ## Considerations & Questions - Do we have existing tests surrounding the **existing** implementations? - If applicable, what is the test coverage like for the existing implementations? - If no existing test coverage is present, what test coverage should be present before refactoring is implemented?
kerem 2026-03-16 15:39:08 +03:00
Author
Owner

@liyasthomas commented on GitHub (Mar 17, 2021):

@JDevx97 I would totally appreciate this refactoring approach.

Do we have existing tests surrounding the existing implementations?

We have a snapshot test for all code generators: https://github.com/hoppscotch/hoppscotch/blob/main/helpers/codegen/tests/codegen.spec.js introduced in #1255

cc: @AndrewBastin

<!-- gh-comment-id:801004537 --> @liyasthomas commented on GitHub (Mar 17, 2021): @JDevx97 I would totally appreciate this refactoring approach. > Do we have existing tests surrounding the existing implementations? We have a snapshot test for all code generators: https://github.com/hoppscotch/hoppscotch/blob/main/helpers/codegen/__tests__/codegen.spec.js introduced in #1255 cc: @AndrewBastin
Author
Owner

@JayceDugan commented on GitHub (Mar 17, 2021):

@liyasthomas Ah brilliant, thank you for those.

That looks promising, without having looked at all generators to confirm if there's any differences on an individual level, if that spec efficiently covers existing functionality for all generators coverage and confirmation should be quite good.

As i'm sure you guys will have better insights than me,

  • Would there be any potential concerns or considerations for implementation?
  • Are there any high priority parts of the application that may be affected as an unintended side effect?
  • Are there any areas of the application these changes might affect, that may not be immediately apparent?

Suggested considerations:

  • Splitting out refactoring of individual generators into isolated branches & PR's to reduce impact during integration and promote constant small additions instead of one large PR to all generators.
  • Recommendations for any naming conventions or methods to be promoted / re-used throughout the system? (Existing or new)
<!-- gh-comment-id:801017254 --> @JayceDugan commented on GitHub (Mar 17, 2021): @liyasthomas Ah brilliant, thank you for those. That looks promising, without having looked at all generators to confirm if there's any differences on an individual level, if that spec efficiently covers existing functionality for all generators coverage and confirmation should be quite good. As i'm sure you guys will have better insights than me, - Would there be any potential concerns or considerations for implementation? - Are there any high priority parts of the application that may be affected as an unintended side effect? - Are there any areas of the application these changes might affect, that may not be immediately apparent? ### Suggested considerations: - Splitting out refactoring of individual generators into isolated branches & PR's to reduce impact during integration and promote constant small additions instead of one large PR to all generators. - Recommendations for any naming conventions or methods to be promoted / re-used throughout the system? (Existing or new)
Author
Owner

@liyasthomas commented on GitHub (Mar 17, 2021):

Would there be any potential concerns or considerations for implementation?

Sticking to modular approach will be exceptionally helpful. Extracting instances of codegen out of main thread is appreciated.

Are there any high priority parts of the application that may be affected as an unintended side effect?

So the code generation part is almost independent of other part of the application. And probably shouldn't interfere with any other functionalities.

Are there any areas of the application these changes might affect, that may not be immediately apparent?

Well, these are the places codegen is being used:

  1. CodegenModal.vue : github.com/hoppscotch/hoppscotch@dc5ca76d05/components/http/CodegenModal.vue
  2. RequestCode() : github.com/hoppscotch/hoppscotch@1042310038/pages/index.vue (L1136)

manual testing on each language generator after implementation will be the best bet to ensure that there's no potential breaking changes.


Splitting out refactoring of individual generators into isolated branches & PR's to reduce impact during integration and promote constant small additions instead of one large PR to all generators.

Agree 💯

Recommendations for any naming conventions or methods to be promoted / re-used throughout the system? (Existing or new)

github.com/hoppscotch/hoppscotch@dc5ca76d05/helpers/codegen/codegen.js

<!-- gh-comment-id:801033784 --> @liyasthomas commented on GitHub (Mar 17, 2021): > Would there be any potential concerns or considerations for implementation? Sticking to modular approach will be exceptionally helpful. Extracting instances of codegen out of main thread is appreciated. > Are there any high priority parts of the application that may be affected as an unintended side effect? So the code generation part is almost independent of other part of the application. And probably shouldn't interfere with any other functionalities. > Are there any areas of the application these changes might affect, that may not be immediately apparent? Well, these are the places codegen is being used: 1. CodegenModal.vue : https://github.com/hoppscotch/hoppscotch/blob/dc5ca76d05c7c3c8c6eac24a23641a2413e5f162/components/http/CodegenModal.vue 2. RequestCode() : https://github.com/hoppscotch/hoppscotch/blob/10423100383f9428d625cc0a023a8ffc07892134/pages/index.vue#L1136 manual testing on each language generator after implementation will be the best bet to ensure that there's no potential breaking changes. --- > Splitting out refactoring of individual generators into isolated branches & PR's to reduce impact during integration and promote constant small additions instead of one large PR to all generators. Agree 💯 > Recommendations for any naming conventions or methods to be promoted / re-used throughout the system? (Existing or new) - Stick to camelCase for function names and variables - Keep all helper functions and utlities inside `/helpers/codegen` : https://github.com/hoppscotch/hoppscotch/tree/dc5ca76d05c7c3c8c6eac24a23641a2413e5f162/helpers/codegen - `codegen.js` contains - List of all code generators - used to display all generators in `CodegenModal.vue`: https://github.com/hoppscotch/hoppscotch/blob/dc5ca76d05c7c3c8c6eac24a23641a2413e5f162/helpers/codegen/codegen.js#L31 - Code general generator function - generate code from `codegenID` and `context`: https://github.com/hoppscotch/hoppscotch/blob/dc5ca76d05c7c3c8c6eac24a23641a2413e5f162/helpers/codegen/codegen.js#L55 https://github.com/hoppscotch/hoppscotch/blob/dc5ca76d05c7c3c8c6eac24a23641a2413e5f162/helpers/codegen/codegen.js
Author
Owner

@JayceDugan commented on GitHub (Mar 18, 2021):

Hey @liyasthomas thanks for the detailed brief, much appreciated. I'll be starting tomorrow night most likely. You're welcome to mark me as the assignee if you like.

Sticking to modular approach will be exceptionally helpful. Extracting instances of codegen out of main thread is appreciated.

Would you at all be able to expand on this in more specifics?

<!-- gh-comment-id:801817914 --> @JayceDugan commented on GitHub (Mar 18, 2021): Hey @liyasthomas thanks for the detailed brief, much appreciated. I'll be starting tomorrow night most likely. You're welcome to mark me as the assignee if you like. > Sticking to modular approach will be exceptionally helpful. Extracting instances of codegen out of main thread is appreciated. Would you at all be able to expand on this in more specifics?
Author
Owner

@liyasthomas commented on GitHub (Mar 18, 2021):

Awesome. I was mentioning about keeping everything related to codegen inside /helpers/codegen.

<!-- gh-comment-id:801821505 --> @liyasthomas commented on GitHub (Mar 18, 2021): Awesome. I was mentioning about keeping everything related to codegen inside `/helpers/codegen`.
Author
Owner

@JayceDugan commented on GitHub (Mar 18, 2021):

Aah gotcha, thank you. Sounds good, I'll be taking a look at the generators as a whole and planning different structures so you can expect the first PR to come through in a few days.

In the meantime thanks again and if you have anything else you'd like me to look across feel free to tag me, more than happy to get involved even if it's just to review suggestions or provide alternative ideas.

<!-- gh-comment-id:801835834 --> @JayceDugan commented on GitHub (Mar 18, 2021): Aah gotcha, thank you. Sounds good, I'll be taking a look at the generators as a whole and planning different structures so you can expect the first PR to come through in a few days. In the meantime thanks again and if you have anything else you'd like me to look across feel free to tag me, more than happy to get involved even if it's just to review suggestions or provide alternative ideas.
Author
Owner

@AndrewBastin commented on GitHub (Mar 30, 2021):

@JayceDugan thanks for the implementation. In a recent commit, we added drop-in support for TypeScript in the project. I think the Codegen system is a good candidate to support TypeScript.

It would be awesome if you could port and continue the work on TypeScript. If not, that is fine too ^_^

<!-- gh-comment-id:810649141 --> @AndrewBastin commented on GitHub (Mar 30, 2021): @JayceDugan thanks for the implementation. In a recent commit, we added drop-in support for TypeScript in the project. I think the Codegen system is a good candidate to support TypeScript. It would be awesome if you could port and continue the work on TypeScript. If not, that is fine too ^_^
Author
Owner

@JayceDugan commented on GitHub (Mar 31, 2021):

Hey @AndrewBastin,

Thanks and that's a brilliant idea, more than happy too.

Also @liyasthomas I've been meaning to get back to you, sorry work has been hectic lately. Released multiple large feature sets in the last two weeks so I've been MIA. I read your comments, including the feedback in the merges & the new branch and definitely agree.

I'll be out of everything by the end of this week and looking forward to continuing with these soon as i'm out.

<!-- gh-comment-id:810657370 --> @JayceDugan commented on GitHub (Mar 31, 2021): Hey @AndrewBastin, Thanks and that's a brilliant idea, more than happy too. Also @liyasthomas I've been meaning to get back to you, sorry work has been hectic lately. Released multiple large feature sets in the last two weeks so I've been MIA. I read your comments, including the feedback in the merges & the new branch and definitely agree. I'll be out of everything by the end of this week and looking forward to continuing with these soon as i'm out.
Author
Owner

@JayceDugan commented on GitHub (Apr 6, 2021):

Hey guys I'm out of things now,

@AndrewBastin Would you by any chance be able to propose a sample structure you might use for the codegens?

I've played around with typescript, played around being the accurate term and wouldn't be able to say I have extensive knowledge, so if you're able to provide any direction around how you may like things or similar structures it'd be much appreciated.

<!-- gh-comment-id:813910514 --> @JayceDugan commented on GitHub (Apr 6, 2021): Hey guys I'm out of things now, @AndrewBastin Would you by any chance be able to propose a sample structure you might use for the codegens? I've played around with typescript, played around being the accurate term and wouldn't be able to say I have extensive knowledge, so if you're able to provide any direction around how you may like things or similar structures it'd be much appreciated.
Author
Owner

@liyasthomas commented on GitHub (Jan 23, 2022):

Refactored the current code generator implementation and released codegen v2: github.com/hoppscotch/hoppscotch@2ec401c766

<!-- gh-comment-id:1019512907 --> @liyasthomas commented on GitHub (Jan 23, 2022): Refactored the current code generator implementation and released codegen v2: https://github.com/hoppscotch/hoppscotch/commit/2ec401c7662d373bff5432f05a84001dcbdbbd15
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
starred/hoppscotch#491
No description provided.