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

fix: ts-node is registered only when it's actually needed #1165

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Gmin2
Copy link
Collaborator

@Gmin2 Gmin2 commented Apr 1, 2024

Description

  • Remove the unconditional registration
  • Created checkForTypescriptFiles and hasTypescriptFilesDir functions to check for the presence of ts files in the filters and hooks directories
  • Conditionally register the ts transpiles in the configureTemplateWorkflow functions using checkForTypescriptFiles.

Related issue(s)
Fixes #1030

Copy link
Contributor

@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.

@Gmin2 Gmin2 marked this pull request as draft April 1, 2024 12:51
Copy link

sonarcloud bot commented Apr 1, 2024

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@Gmin2 Gmin2 marked this pull request as ready for review April 4, 2024 07:03
@Gmin2
Copy link
Collaborator Author

Gmin2 commented Apr 5, 2024

can you take a look @derberg @Florence-Njeri

@@ -11,10 +11,52 @@ const { isAsyncFunction } = require('./utils');
* @param {String} filtersDir Directory where local filters are located.
*/
module.exports.registerFilters = async (nunjucks, templateConfig, templateDir, filtersDir) => {
const hasTypescriptFiles = await checkForTypescriptFiles(templateDir, filtersDir);
if(hasTypescriptFiles) {
utils.registerTypescript(true)
Copy link
Collaborator

@lmgyuan lmgyuan Apr 7, 2024

Choose a reason for hiding this comment

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

In my IDE, the utils seems to be undefined. You probably want to add const { registerTypescript } = require('./utils');
And then in line 16, use registerTypescript(true) instead

Copy link
Member

Choose a reason for hiding this comment

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

yup, @utnim2 please fix line 4 that for now requires only isAsyncFunction

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.

can you explain why you think your solution must be implemented instead of what was suggested in #1030 (comment)

wouldn't it just be easier to plug into the existing code that anyway "walks" over filters and hooks files and you can use it to figure if ts file is there or not?

@Gmin2
Copy link
Collaborator Author

Gmin2 commented Apr 10, 2024

can you explain why you think your solution must be implemented instead of what was suggested in #1030 (comment)

wouldn't it just be easier to plug into the existing code that anyway "walks" over filters and hooks files and you can use it to figure if ts file is there or not?

The suggested solution iterates through all files even there are no Typescript files present,my solution skips the registration process altogether if no Typescript file is present
I might be wrong here 🤔

@lmgyuan
Copy link
Collaborator

lmgyuan commented Apr 11, 2024

can you explain why you think your solution must be implemented instead of what was suggested in #1030 (comment)
wouldn't it just be easier to plug into the existing code that anyway "walks" over filters and hooks files and you can use it to figure if ts file is there or not?

The suggested solution iterates through all files even there are no Typescript files present,my solution skips the registration process altogether if no Typescript file is present I might be wrong here 🤔

If I understand what @derberg said correctly, I think regardless of whether hasTypeScript is true or not in:

const hasTypescriptFiles = await this.checkForTypescriptFiles(); if(hasTypescriptFiles) { utils.registerTypeScript(true); }

This part of the code in registerLocalFilters will also be executed:

walker.on('file', async (root, stats, next) => {
      try {
        const filePath = path.resolve(templateDir, path.resolve(root, stats.name));
        // If it's a module constructor, inject dependencies to ensure consistent usage in remote templates in other projects or plain directories.
        delete require.cache[require.resolve(filePath)];
        const mod = require(filePath);

        addFilters(nunjucks, mod);
        
        next();
      } catch (e) {
        reject(e);
      }
    });

which goes through the directories where filters are located. It is the same case for hooks. Therefore, instead of checking it in a different place, it might be preferable to just look for TS files when looking for filters and hooks, to make the codebase clearer and more concise.

They are my understandings. Please correct me if any of it is incorrect : )

@derberg
Copy link
Member

derberg commented Apr 11, 2024

@lmgyuan yes, this is exactly it, thanks. We anyway iterate there, thus better to inject TS validation there rather than have another iteration only for TS. Better performance

@Gmin2
Copy link
Collaborator Author

Gmin2 commented Apr 11, 2024

@lmgyuan yes, this is exactly it, thanks. We anyway iterate there, thus better to inject TS validation there rather than have another iteration only for TS. Better performance

Ok making the changes

@derberg
Copy link
Member

derberg commented Apr 22, 2024

do you need some help @utnim2 ?

@Gmin2 Gmin2 reopened this May 18, 2024
Copy link

sonarcloud bot commented Jun 15, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

sonarcloud bot commented Jun 20, 2024

@Gmin2
Copy link
Collaborator Author

Gmin2 commented Jun 29, 2024

This is ready for review and merge

cc @derberg

Copy link

sonarcloud bot commented Jul 3, 2024

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.

there is still a linter issue we discussed during the call, and as you can see GitHub action fails because of it

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

Successfully merging this pull request may close these issues.

ts-node is registered if generator is imported in a .js file
3 participants