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

fix (core): allow falsy values in astro config integrations arrays #4427

Merged
merged 2 commits into from
Aug 23, 2022

Conversation

cameronmcefee
Copy link
Contributor

Changes

TL;DR: this updates the astro config type to allow falsy values in the integrations array, which the docs indicate should be possible. Related: withastro/docs#1334


The docs state:

Falsy integrations are ignored, so you can toggle integrations on & off without worrying about left-behind undefined and boolean values.

integrations: [
  // Example: Skip building a sitemap on Windows
  process.platform !== 'win32' && sitemap()
]

However, Typescript will generate the following error if you try to do this:

Type 'false | AstroIntegration' is not assignable to type 'AstroIntegration | AstroIntegration[]'.

In this particular change, I've chosen a nullish interpretation of "falsy" allowing false, null, and undefined. I'm happy to update to a more conservative false only solution, which might read more cleanly.

Based on the schema in https://github.com/withastro/astro/blob/main/packages/astro/src/core/config.ts#L139, I believe this is a safe change, as the looser type will still conform to the validation format due to the filter(Boolean) preprocessing.

I'm in a bit over my head with the contribution formula here, so holler at me if I've done something wrong or there's a better way to handle this.

Testing

Tests are not included because I couldn't get them to fully run locally with a fresh install prior to making this change. I'm hoping the PR checks will run the appropriate checks, at which point I'll update.

Docs

There are no changes to the docs, as this change brings the codebase inline with what the docs already state.

@changeset-bot
Copy link

changeset-bot bot commented Aug 22, 2022

🦋 Changeset detected

Latest commit: 6af81c3

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

This PR includes changesets to release 14 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/error-react-spectrum Patch
@e2e/error-sass Patch
@e2e/errors Patch
@e2e/hydration-race Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/solid-recurse Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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 Aug 22, 2022
@matthewp
Copy link
Contributor

We have an existing test that the functionality works:

it('ignores falsey "integration" values', async () => {
const result = await validateConfig(
{ integrations: [0, false, null, undefined] },
process.cwd()
);
expect(result.integrations).to.deep.equal([]);
});

So this PR simply fixes the types. Thank you.

@matthewp matthewp merged commit b2e976f into withastro:main Aug 23, 2022
@astrobot-houston astrobot-houston mentioned this pull request Aug 23, 2022
@cameronmcefee cameronmcefee deleted the fix-core/match-type-to-docs branch August 23, 2022 16:53
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.

None yet

2 participants