-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix(assets): Fix endpoint not being injected when an integration would enable experimental.assets
#7829
Conversation
…d enable `experimental.assets` for the user
🦋 Changeset detectedLatest commit: a68eb72 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Erika! LGTM. Plus I trust you.
@@ -89,6 +91,13 @@ class AstroBuilder { | |||
command: 'build', | |||
logging, | |||
}); | |||
|
|||
// HACK: Since we only inject the endpoint if `experimental.assets` is on and it's possible for an integration to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth a “Remove in 3.0” comment or would we pick it up anyway because experimental.assets
will go away so this would be a type error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 3.0 it's likely that we'll need an option to disable the endpoint (ex: maybe you have your own, maybe you don't want any assets
stuff), so we'll still need to check for something in the config still here. So for now I'll leave it like this and update whenever we'll add that option
Changes
What the title says. We'd only check for
experimental.assets
before running the integration's config setup hooks, so we'd miss it if an integration would enable it for the user (such as StarlightTesting
Tested manually, we'll be removing this in 3.0 anyway, so
Docs
N/A