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

refactor: rework supportedAstroFeatures #11806

Merged
merged 7 commits into from
Sep 13, 2024
Merged

Conversation

Princesseuh
Copy link
Member

Changes

supportedAstroFeatures on adapters is quite static, and the reality is often more nuanced. For instance, for Sharp, sometimes it's not supported with the default configuration, but with certain config it can be, but even then, it might only be on pre-rendered pages etc.

This PR does a few things:

  • Remove the assets section,
  • Add a new "limited" value
  • Allow adapters to give a custom message in addition to the default one
  • Refactor so the logging all happen in one place, it was a bit confusing how it was shared between the hook and the validation function

Testing

Updated tests, and the current ones should pass

Docs

Will do!

Copy link

changeset-bot bot commented Aug 21, 2024

🦋 Changeset detected

Latest commit: d835c43

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

supportKind: AdapterSupport,
adapterMessage?: string,
) {
switch (supportKind) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Merged the different functions into one here that way you get an exhaustive check on the switch, felt nicer

Comment on lines +63 to +64
export type AdapterSupportsKind =
(typeof AdapterFeatureStability)[keyof typeof AdapterFeatureStability];
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the source of truth from the type to the runtime check here, that way they're always accurate

}
if (supportValue === AdapterFeatureStability.STABLE) {
return true;
} else if (hasCorrectConfig()) {
Copy link
Member Author

@Princesseuh Princesseuh Aug 21, 2024

Choose a reason for hiding this comment

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

The previous check here was weird, it only checked if the config was correct for the unsupported value, but you need to always check it, otherwise you get warning for every experimental feature, even the ones you don't use?

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Aug 21, 2024
Copy link
Member

@alexanderniebuhr alexanderniebuhr left a comment

Choose a reason for hiding this comment

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

We have moved the adapter to https://github.com/withastro/adapters, so this PR can't be merged here anymore.
I would offer to port over the changes to the new repo for you, just let me know if you want me to do that.

@Princesseuh Princesseuh changed the base branch from next to feat/remove-hybrid September 3, 2024 10:14
@alexanderniebuhr
Copy link
Member

@Princesseuh do you want me to port over the changes of the adapter related code, or are you going to do it yourself?

Base automatically changed from feat/remove-hybrid to next September 6, 2024 23:07
@github-actions github-actions bot added docs pr and removed pkg: integration Related to any renderer integration (scope) labels Sep 12, 2024
@Princesseuh
Copy link
Member Author

ill do it myself, no worries

@github-actions github-actions bot added the semver: minor Change triggers a `minor` release label Sep 12, 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.

@Princesseuh Princesseuh marked this pull request as ready for review September 12, 2024 23:23
@Princesseuh Princesseuh dismissed alexanderniebuhr’s stale review September 12, 2024 23:23

will port the changes to the adapters repo

@Princesseuh Princesseuh merged commit f7f2338 into next Sep 13, 2024
13 of 14 checks passed
@Princesseuh Princesseuh deleted the refactor/supportAstroFeatures branch September 13, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr 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.

3 participants