Skip to content

feat: add support for configurable hooks libraries - #334

Merged
derberg merged 8 commits into
asyncapi:masterfrom
derberg:refactor-hooks
May 14, 2020
Merged

feat: add support for configurable hooks libraries#334
derberg merged 8 commits into
asyncapi:masterfrom
derberg:refactor-hooks

Conversation

@derberg

@derberg derberg commented May 12, 2020

Copy link
Copy Markdown
Member

Description

  • generate:before and others become a hook type. Hook name is the name of hook function. you do not have to always provide it but it is a must have for hooks libararies and you enable a hook by referring it's name
  • Way of writing hook function changed. register function is no longer needed as part of the hook which enables easier testing. You write a regular functon and expose it grouped by hook type:
module.exports = {
  "generate:after": [myHook1, myHook2],
  "generate:before": myHook3
}
  • hooks registration extracted to separate js file and local hooks traversing refactored to use xfs for async walking as we use in other parts of the generator
  • test and docs
  • I tried to have more sophisticated tests for hooks registration where I can test different scenarios when passing different configs with errors or not but I think I'm too stupid to do it with jest. I wasted a few hours trying to mock files with modules that could be used in the registration but first I stumbled upon an issue with xfs that didn't work well with mock-fs. Then with quick refactor to stop using xfs in favor of klaw things were getting better but this would require larger refactor to kind out xfs from the project.

MIGRATION PATHS
This PR is a breaking change and you need to adjust your templates hooks, if you have one. There are 2 paths to follow here:

Related issue(s)
See also #318

@derberg derberg changed the title Refactor hooks feat: add support for configurable hooks libraries May 12, 2020
@derberg
derberg requested a review from fmvilas May 12, 2020 14:29
Tenischev
Tenischev previously approved these changes May 12, 2020
Comment thread README.md Outdated
Comment thread cli.js Outdated
Comment thread docs/authoring.md Outdated
Comment thread docs/authoring.md
]
],
"hooks": {
"@asyncapi/generator-hooks": "hookFunctionName"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Will it be better to provide upper in example section at least one hook with defined name?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What do you mean? in example section we provide examples of hooks that you rather put locally in your template, writing own hooks library is like advanced usage in my opinion

this config example shows usage of our official hooks library, you do not have to add to config your local hooks

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but assume i'm newbie. I just read Hooks section, now read this section and see specify exact name of the hook. And now i ask, what exactly hook name is? This has not been mentioned before.
So, sure, you do not need to give names to local hooks, but example of hook with name will increase understanding of the concept, from my point of view.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

how about now? I added some more content 87a9dfd

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still think that one example is better than long description 😄 , but nevertheless questions are described now in docs. So it's fine.

fmvilas
fmvilas previously approved these changes May 13, 2020

@fmvilas fmvilas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good. Just left a suggestion and agree with @Tenischev comments but overall looks great.

Regarding tests, I think we should introduce integration (snapshot maybe) tests for these features, would be easier than doing unit tests. Not now though.

Comment thread docs/authoring.md Outdated
@derberg
derberg dismissed stale reviews from fmvilas and Tenischev via fb6d598 May 13, 2020 08:48
Tenischev
Tenischev previously approved these changes May 13, 2020
Comment thread docs/authoring.md
]
],
"hooks": {
"@asyncapi/generator-hooks": "hookFunctionName"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I still think that one example is better than long description 😄 , but nevertheless questions are described now in docs. So it's fine.

fmvilas
fmvilas previously approved these changes May 14, 2020
@derberg
derberg dismissed stale reviews from fmvilas and Tenischev via c962be2 May 14, 2020 09:43
@sonarqubecloud

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@derberg
derberg merged commit 4af34b1 into asyncapi:master May 14, 2020
@derberg
derberg deleted the refactor-hooks branch May 14, 2020 15:17
@asyncapi-bot

Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 0.46.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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants