Skip to content

Conversation

@altano
Copy link
Contributor

@altano altano commented Apr 24, 2025

Changes

Regarding experimental fonts, the documentation says:

To disable fallback fonts completely, configure an empty array:

fallbacks: []

But I get a Typescript error if I do that. This is a simple typing issue, not a behavioral one, so in this PR I am:

  • fixing the type so an empty array is allowed, to match the doc

Closes #13659

Testing

Added a type test that fails before this change and succeeds after:

BEFORE:

$ pnpm test:types
test/types/define-config.ts:150:8 - error TS2322: Type '[]' is not assignable to type '[string, ...string[]]'.
Source has 0 element(s) but target requires 1.

150 fallbacks: [],
~~~~~~~~~

AFTER:

$ pnpm test:types
succeeds

Docs

Bringing types in line with docs, no doc updates needed

@changeset-bot
Copy link

changeset-bot bot commented Apr 24, 2025

🦋 Changeset detected

Latest commit: 00926a2

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 the pkg: astro Related to the core `astro` package (scope) label Apr 24, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Apr 24, 2025

CodSpeed Performance Report

Merging #13681 will not alter performance

Comparing altano:fonts-fix-fallbacks-type (00926a2) with main (6ed8360)

Summary

✅ 6 untouched benchmarks

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

I'm not sure about the nature of this fix. I recall that having an empty array for fallback must be an error. Do we have a configuration test for this case? Can you check @altano, and make sure that fallback: [] throws an error?

cc @florian-lefebvre

Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

@florian-lefebvre
Copy link
Member

@ematipico empty fallbacks should not throw an error, this is a proper bug

@altano altano force-pushed the fonts-fix-fallbacks-type branch from 7951c02 to 362a7a2 Compare April 27, 2025 01:31
@altano
Copy link
Contributor Author

altano commented Apr 27, 2025

Can you also add a test case here?

Done, PR updated. But let me know if you had something more in mind, as I just added a basic "validateConfig shouldn't throw for empty fallbacks array" as a test:

assert.doesNotThrow(() =>
	validateConfig({
		experimental: {
			fonts: [
				{
					provider: fontProviders.google(),
					name: 'Roboto',
					fallbacks: [],
					cssVariable: '--font-roboto',
				},
			],
		},
	}),
);

Documentation says:

> To disable fallback fonts completely, configure an empty array:
>
> fallbacks: []

But I get a Typescript error if I do that. This is a simple typing issue, not a behavioral one, so in this PR I am:

- fixing the type so an empty array is allowed, to match the doc
- adding a type test that fails before this change and succeeds after:

> test/types/define-config.ts:150:8 - error TS2322: Type '[]' is not assignable to type '[string, ...string[]]'.
>   Source has 0 element(s) but target requires 1.
>
> 150        fallbacks: [],
>            ~~~~~~~~~

Partially addresses withastro#13659
@altano altano force-pushed the fonts-fix-fallbacks-type branch from 362a7a2 to 042ffc1 Compare April 27, 2025 01:33
Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

Thanks!

@florian-lefebvre florian-lefebvre merged commit 6d0e382 into withastro:main Apr 27, 2025
17 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Apr 27, 2025
openscript pushed a commit to openscript/astro that referenced this pull request Sep 12, 2025
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)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

experimental font feedback

3 participants