fix: experimental svg types #12625
fix: experimental svg types #12625
Conversation
🦋 Changeset detectedLatest commit: 7c0a1b4 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
bot
added
pkg: astro
| @@ -22,7 +22,7 @@ export type SvgRenderMode = 'inline' | 'sprite'; | |||
| export function makeSvgComponent( | |||
| meta: ImageMetadata, | |||
| contents: Buffer | string, | |||
| options?: { mode?: SvgRenderMode }, | |||
| options?: { mode: SvgRenderMode }, | |||
There was a problem hiding this comment.
Having an optional mode should be a bug. Users should either provide a boolean, or provide an object with explicit mode
| return svgConfig ? ASTRO_CONFIG_DEFAULTS.experimental.svg : undefined; | ||
| return svgConfig | ||
| ? { | ||
| mode: 'inline' as SvgRenderMode, |
There was a problem hiding this comment.
This cast make TS happy
| @@ -4,6 +4,6 @@ import { defineConfig } from 'astro/config'; | |||
| export default defineConfig({ | |||
| integrations: [mdx()], | |||
| experimental: { | |||
| svg: {} | |||
There was a problem hiding this comment.
This way of passing the option shouldn't be valid
There was a problem hiding this comment.
IIUC passing {} is like passing true. Couldn't we support {} without specifying mode too? It feels to me futureproofing if it ever has more optional options in the object, so you don't have to specify mode always.
There was a problem hiding this comment.
We can do everything, but what does svg: {} even mean to the user and to us? I don't want to be pedantic, I'll see what I can do
There was a problem hiding this comment.
I applied the change in 26c65f2 (#12625) and tested that svg: {} is still a valid configuration
There was a problem hiding this comment.
It doesn't mean a lot currently, but it's futureproofing if we add more options to it in the future, so I think it's an ok tradeoff for now as long as we don't recommend people to enable with {} only.
CodSpeed Performance ReportMerging #12625 will degrade performances by 21.79%Comparing Summary
Benchmarks breakdown
|
ematipico
force-pushed
the
chore/svg-experimental-type
branch
from
26c65f2 to
7c0a1b4
Compare
Changes
Closes #12626
This PR fixes an issue where
experimental.svghad incorrect types.The previous types allowed having an empty object
{}but it failed with a boolean value, which how we document it.This was reported on Discord: https://discord.com/channels/830184174198718474/1313850696126431263
Testing
There was a failing test, which is now fixed
Docs