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: Refactor integration hooks type #11304

Merged
merged 6 commits into from
Jul 17, 2024

Conversation

Fryuni
Copy link
Member

@Fryuni Fryuni commented Jun 20, 2024

Changes

astro

  • Extract the AstroIntegration['hooks'] type into a global Astro.IntegrationHooks interface to allow for third-party extensions.

As discussed on Discord, this breaks only a very narrow use case that wasn't documented as being supported and relied on the hooks not being extensible in order to create an extension workaround on top of it. So it is not being considered as a breaking change.

@astrojs/db

  • Remove the AstroDbIntegration type (BREAKING)
  • Kept the defineDbIntegration just so people don't have to change their code for that, it is no longer needed

Testing

This doesn't change any runtime behavior

Docs

We can now document how integrations can add their own hooks 😄

Future scope

This PR only allows integrations to declare the types for their hooks; it doesn't provide any higher-level mechanism to trigger those hooks. Triggering them is already possible with the following code:

export default myIntegration: (): AstroIntegration => {
  let integrations: AstroIntegration[] = [];

  return {
    name: 'myLib',
    hooks: {
	  'astro:config:setup': ({config}) => {
	    integrations = config.integrations;
	  },
	  // Wherever some other logic runs
	  'astro:build:done': () => {
	    for (const integration of integrations) {
	      integrations.hooks['myLib:eventHappened']?.(hookParameters);
	    }
	  },
    },
  };
}

In the future, we might provide some way for integrations to trigger their hooks with less boilerplate

@Fryuni Fryuni self-assigned this Jun 20, 2024
Copy link

changeset-bot bot commented Jun 20, 2024

🦋 Changeset detected

Latest commit: 13e2244

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

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

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Jun 20, 2024
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.

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@ematipico ematipico added this to the 4.12.0 milestone Jun 20, 2024
.changeset/curvy-otters-jog.md Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Show resolved Hide resolved
@@ -0,0 +1,18 @@
---
'astro': minor
'@astrojs/db': minor
Copy link
Member

Choose a reason for hiding this comment

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

We should create a new changeset only for @astrojs/db that shows the migration path of the new type. Since this is a breaking change, I believe this should stand out in the changelog

Copy link
Member

Choose a reason for hiding this comment

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

Just noting that I'll review here when I have a docs PR!

Copy link
Member Author

Choose a reason for hiding this comment

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

It is barely even a breaking change for @astrojs/db though. It breaks something we never said was there (the AstroDbIntegration type). The helper defineDbIntegration method shown on the docs continues to work with no changes.

The main difference is requiring the new version of astro.

But I'll write that up either way

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Just leaving a placeholder for docs! When I have a docs PR submitted, I can review it and this!

@@ -0,0 +1,18 @@
---
'astro': minor
'@astrojs/db': minor
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that I'll review here when I have a docs PR!

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Quick review for docs! Looks great, I am mostly suggesting a more "use case" description to kick this off!

Additionally, there is another change in here to @astrojs/db that will need a corresponding docs update, as this type being removed is currently documented.

.changeset/curvy-otters-jog.md Outdated Show resolved Hide resolved
.changeset/curvy-otters-jog.md Outdated Show resolved Hide resolved
.changeset/curvy-otters-jog.md Outdated Show resolved Hide resolved
.changeset/curvy-otters-jog.md Outdated Show resolved Hide resolved
@Fryuni
Copy link
Member Author

Fryuni commented Jul 6, 2024

Additionally, there is another change in here to @astrojs/db that will need a corresponding docs update, as this type being removed is currently documented.

It is not documented Sarah, AstroDbIntegration is nowhere in the docs. The defineDbIntegration utility is documented and that is why I kept it around.

.changeset/blue-colts-film.md Outdated Show resolved Hide resolved
.changeset/blue-colts-film.md Show resolved Hide resolved
packages/db/src/core/load-file.ts Show resolved Hide resolved
packages/db/src/core/types.ts Show resolved Hide resolved
@sarah11918
Copy link
Member

@Fryuni Sorry, I thought I checked closely, but I was concerned about this example which you're right, is not the type itself. As long as this example doesn't need updating, then no docs are needed, you're correct!

image

@sarah11918
Copy link
Member

Just pinging @Fryuni to look at my changeset suggestions! The Docs PR has already been approved and is ready for next week, so this is just tiny details now! 🙌

@sarah11918 sarah11918 dismissed their stale review July 12, 2024 11:23

no longer blocked by docs!

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Approving for docs!

@matthewp matthewp merged commit 2e70741 into main Jul 17, 2024
14 checks passed
@matthewp matthewp deleted the fryuni/astro-integration-hooks-interface branch July 17, 2024 15:59
@astrobot-houston astrobot-houston mentioned this pull request Jul 17, 2024
ematipico pushed a commit that referenced this pull request Jul 18, 2024
* feat: Refactor integration hooks type

* Revert formatting changes

* More detailed changelog

* Add changeset for Astro DB

* Apply suggestions from code review

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

---------

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@astrobot-houston astrobot-houston mentioned this pull request Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants