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

Merged
merged 13 commits into from
Jul 31, 2024

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

sonarqubecloud 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 force-pushed the ts-node branch 2 times, most recently from 2924a25 to f133ecd Compare May 18, 2024 14:53
@Gmin2 Gmin2 closed this May 18, 2024
@Gmin2 Gmin2 reopened this May 18, 2024
Copy link

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

@Gmin2
Copy link
Collaborator Author

Gmin2 commented Jun 29, 2024

This is ready for review and merge

cc @derberg

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

@derberg
Copy link
Member

derberg commented Jul 29, 2024

@Gmin2 you need to solve conflicts

Copy link

changeset-bot bot commented Jul 31, 2024

🦋 Changeset detected

Latest commit: cb84886

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@asyncapi/generator Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Gmin2 Gmin2 requested a review from Florence-Njeri as a code owner July 31, 2024 12:43
Copy link

@derberg
Copy link
Member

derberg commented Jul 31, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit 44fcc33 into asyncapi:master Jul 31, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Archive
Development

Successfully merging this pull request may close these issues.

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