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: add TypeScript support for hooks and filters #519

Merged
merged 15 commits into from
Mar 5, 2021

Conversation

magicmatatjahu
Copy link
Member

@magicmatatjahu magicmatatjahu commented Mar 1, 2021

Description

Add TypeScript supports in hooks and Nunjucks's filters:

  • this PR only enables TS support for hooks/filters. Support for React will be added here Support TS in sdk generator-react-sdk#3
  • code for TS is transpiling on-the-fly so developer doesn't have to transpile it before publishing to npm, but by this we must include ts-node, typescript and source-map packages in generator.
  • add docs with general assumptions for using TS in hooks/filters.

Related issue(s)
Resolve #455

@magicmatatjahu magicmatatjahu added the enhancement New feature or request label Mar 1, 2021
@magicmatatjahu magicmatatjahu marked this pull request as ready for review March 1, 2021 11:52
@magicmatatjahu magicmatatjahu changed the title feat: add support for TS feat: add TS support for hooks and filters Mar 1, 2021
@fmvilas
Copy link
Member

fmvilas commented Mar 4, 2021

I'm afraid this is introducing much more complexity to the Generator, especially the transpilation process. What need exactly is driving this PR?

@magicmatatjahu
Copy link
Member Author

@fmvilas How complexity? It only transpiles TS code on the fly when we import hooks or filters in hook/filter registries - user doesn't need to transpile anything before rendering template :) Maybe misunderstanding for issue and PR is that I missed to link to another issue in generator-react-sdk - asyncapi/generator-react-sdk#3 If we will have a TS support in react renderer, then we will have inconsistencies because hooks will have to be written in JS and the template itself will be written entirely in TS. Maybe TS support for filters is not needed, but this PR is only a preparation for TS support for React templates.

@fmvilas
Copy link
Member

fmvilas commented Mar 4, 2021

Oh, ok 👍 I was missing that context. Thanks!

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.

@magicmatatjahu great work, only few comments, all docs related

docs/authoring.md Outdated Show resolved Hide resolved
- Installing the `typescript` package and creating the` tsconfig.json` file isn't necessary.
- Source code of the hook/filter must have `.ts` extension.
- Each package related with the typings for TypeScript like `@types/node` must be installed in the template under `dependencies` array. This is because the Generator transpiles the TypeScript code on-the-fly while rendering the template, so cannot uses packages under `devDependencies`.
- Each package should have installed `@types/node` package to support typings for Node.
Copy link
Member

Choose a reason for hiding this comment

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

what package?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, here should be template -> Each template...

lib/utils.js Show resolved Hide resolved
@@ -98,6 +98,11 @@ async function registerConfigHooks(hooks, templateDir, templateConfig) {
* @param {Array} config List of hooks configured in template configuration
*/
function addHook(hooks, mod, config) {
// for ESM `export default {}`
Copy link
Member

Choose a reason for hiding this comment

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

what does it mean, what esm has to do with this part?

Copy link
Member Author

Choose a reason for hiding this comment

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

For example. If you write export for hooks in CommonJS, then you have:

module.exports = {
  'generate:before': (generator) => {
  }
};

But if you are using ESM in TS, you can write:

export default {
  'generate:before': (generator) => {
  }
}

Unfortunately, then it is transpiled to:

module.exports = {
  default: {
      'generate:before': (generator) => {
      }
  }
};

Copy link
Member

Choose a reason for hiding this comment

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

oh, ok, you just made a comment to explain the if. Can you make it more descriptive 🙏🏼 I totally didn't map it in my head, probably because I use ESM very rarely. So please write this comment for people like me too 😄

@magicmatatjahu magicmatatjahu requested a review from derberg March 5, 2021 10:16
docs/authoring.md Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg derberg changed the title feat: add TS support for hooks and filters feat: add TypeScript support for hooks and filters Mar 5, 2021
@magicmatatjahu magicmatatjahu merged commit 0ee1805 into asyncapi:master Mar 5, 2021
@magicmatatjahu magicmatatjahu deleted the ts-support branch March 5, 2021 13:05
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 1.5.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
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TS in hooks and filters
4 participants