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 hooks library to generator #1304

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

derberg
Copy link
Member

@derberg derberg commented Oct 14, 2024

Resolves #1270

I added inline comments for some more context on the PR

What this PR does? from the changeset:

  • Package @asyncapi/generator-hooks is now part of generator repo and won't be released separately. Source code is stored under apps/hooks but the package/library name stays as it was originally for backward compatibility,
  • By default @asyncapi/generator-hooks package, known as package with a lot of different hooks used in templates, is available in the generator and you no longer have to configure it in your package.json in dependencies. Package @asyncapi/generator-hooks will no longer be published to NPM separately and is deprecated. You can still have your own hooks, and you can still store them in a separate package and configure with your template,
  • Remember that the fact that hooks package is now included by default, doesn't mean all hooks from it are enabled by default. You still have to enable given hook in the configuration explicitly because some hooks can execute automatically without passing a specific parameter. Also hook's supported parameters also need to be defined in your template's config.

Also, next steps after it is merged:

Copy link

changeset-bot bot commented Oct 14, 2024

🦋 Changeset detected

Latest commit: 42493f2

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

@@ -0,0 +1,7 @@
---
Copy link
Member Author

@derberg derberg Oct 15, 2024

Choose a reason for hiding this comment

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

adding changeset as we need to release a new 2.5 version

have a look if all is clear

README.md Outdated

1. [Nunjucks-filters](apps/nunjucks-filters): This library contains generator filters that can be reused across multiple templates, helping to avoid redundant work. These filters are designed specifically for Nunjucks templates and are included by default with the generator, so there's no need to add them to dependencies seprately.
Copy link
Member Author

Choose a reason for hiding this comment

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

don't worry 1. is always used, it is markdown feature, so you do not have to worry on proper order of numbers, see rendered version: https://github.com/asyncapi/generator/blob/aa35277c7384a2b33767cd8706da615bdcbd1292/README.md


> warning: This package doesn't support AsyncAPI 1.x anymore. We recommend to upgrade to the latest AsyncAPI version using the [AsyncAPI converter](https://github.com/asyncapi/converter-js) (You can refer to [installation guide](/apps/generator//docs//installation-guide.md)). If you need to convert documents on the fly, you may use the [Node.js](https://github.com/asyncapi/converter-js) or [Go](https://github.com/asyncapi/converter-go) converters.
Copy link
Member Author

Choose a reason for hiding this comment

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

v1 of asyncapi was ages before, better remove it as it only confuse instead of helping

}
}
```
1. Some hooks support custom parameters that template's user can use to specify different behaviour of the hook. To enable these, you need to also add them to the list of your template's parameters:
Copy link
Member Author

Choose a reason for hiding this comment

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

expect(file).toMatchSnapshot();
const mdFile = await readFile(path.join(outputDir, testOutputFile), 'utf8');
//react template has hooks lib enabled and generation of asyncapi document that was passed as input should work out of the box without adding @asyncapi/generator-hooks to dependencies
const asyncAPIFile = await readFile(path.join(outputDir, 'asyncapi.yaml'), 'utf8');
Copy link
Member Author

Choose a reason for hiding this comment

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

basically test is extended and making sure asyncapi document also gets into output because the proper hook (without adding it to dependency) is configured

not sure it makes sense to validate in test, if it really is not added to dependencies 🤔

Copy link
Collaborator

@Gmin2 Gmin2 Oct 16, 2024

Choose a reason for hiding this comment

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

not sure it makes sense to validate in test, if it really is not added to dependencies 🤔

not sure why we need to check for asyncapi document is in output ?

Copy link
Member Author

Choose a reason for hiding this comment

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

already answered in a meeting

for others: it is needed as before this PR, and in general, by default, generator do not output asyncapi document that it received in input. There is this generic hook for it, and it is added to our test template with this PR - so we need to make sure in test we see it in output - to confirm hooks integation works

@@ -0,0 +1,37 @@
{
Copy link
Member Author

@derberg derberg Oct 15, 2024

Choose a reason for hiding this comment

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

@@ -0,0 +1,33 @@
const fs = require('fs');
Copy link
Member Author

@derberg derberg Oct 15, 2024

Choose a reason for hiding this comment

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

@@ -8,6 +8,8 @@
"test": "turbo run test",
"lint": "turbo lint",
"lint:fix": "turbo run lint:fix",
"generate:assets": "turbo run generate:assets && npm run generate:readme:toc",
Copy link
Member Author

Choose a reason for hiding this comment

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

during work on this template, when I added new section in readme I noticed TOC is not updated - we forgot about enabling TOC update automation when monorepo was included - not it is done, also generator api docs refresh is also now fixed

@@ -1,5 +1,5 @@
{
"$schema": "https://turbo.build/schema.json",
"$schema": "https://turbo.build/schema.v1.json",
Copy link
Member Author

Choose a reason for hiding this comment

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

we were pointing to "latest" version and with turborepo v2 schema was showing errors (pipeline prop is not in v2)

@asyncapi-bot asyncapi-bot added the bounty AsyncAPI Bounty program related label label Oct 16, 2024
@aeworxet
Copy link

@asyncapi/bounty_team

magicmatatjahu
magicmatatjahu previously approved these changes Oct 21, 2024
Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Member Author

derberg commented Oct 21, 2024

@Florence-Njeri can you have a look on docs changes?

Copy link
Collaborator

@Florence-Njeri Florence-Njeri left a comment

Choose a reason for hiding this comment

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

Hey @derberg I left a few editorial reviews on the docs. Lmk :)

.changeset/hooks-in.md Outdated Show resolved Hide resolved
.changeset/hooks-in.md Outdated Show resolved Hide resolved
.changeset/hooks-in.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
apps/generator/docs/configuration-file.md Outdated Show resolved Hide resolved
apps/generator/docs/hooks.md Outdated Show resolved Hide resolved
apps/generator/docs/hooks.md Outdated Show resolved Hide resolved
apps/generator/docs/hooks.md Outdated Show resolved Hide resolved
@derberg
Copy link
Member Author

derberg commented Oct 21, 2024

@Florence-Njeri thanks for detailed review, all changes applied!

Copy link

@derberg
Copy link
Member Author

derberg commented Oct 30, 2024

/rtm

@asyncapi-bot asyncapi-bot merged commit 2d16234 into asyncapi:master Oct 30, 2024
12 checks passed
@derberg derberg deleted the integratehooks branch December 11, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty AsyncAPI Bounty program related label ready-to-merge
Projects
Status: Completed
Status: Archive
Development

Successfully merging this pull request may close these issues.

Move hooks inside monorepo
6 participants