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

Support passing inline Astro config to getViteConfig() #10963

Merged
merged 3 commits into from
May 8, 2024

Conversation

delucis
Copy link
Member

@delucis delucis commented May 6, 2024

Changes

Adds support for passing an inline Astro configuration object to getViteConfig()

If you are using getViteConfig() to configure the Vitest test runner, you can now pass a second argument to control how Astro is configured. This makes it possible to configure unit tests with different Astro options when using Vitest’s workspaces feature.

// vitest.config.ts
import { getViteConfig } from 'astro/config';

export default getViteConfig(
  /* Vite configuration */
  { test: {} },
  /* Astro configuration */
  {
    site: 'https://example.com',
    trailingSlash: 'never',
  },
);

Testing

Applied as a patch in Starlight and tested using i18n features in a unit test. It worked as expected.

Docs

Docs PR: withastro/docs#8192

Copy link

changeset-bot bot commented May 6, 2024

🦋 Changeset detected

Latest commit: d81b459

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 pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels May 6, 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.

@ematipico
Copy link
Member

I have mixed feelings about this one. I was planning to use this function for the container APIs, which will also accept a config.

If this function also accepts a config, it would break the compatibility. Why do we pass the inline config instead of user config? Why isn't this validated?

@delucis
Copy link
Member Author

delucis commented May 6, 2024

Why do we pass the inline config instead of user config? Why isn't this validated?

I’m not sure I understand the first question here?

We currently document using getViteConfig() to support Vitest. However, the resolveConfig() inside getViteConfig() doesn’t reliably work for testing, especially for projects like Starlight where we need to test different Astro configuration options.

In this case, @HiDeoo is working on a refactor to use astro:i18n, but astro:i18n throws an error if the i18n options are not set. Currently we have no way to set Astro config at all in tests, firstly because resolveConfig() finds no astro.config.mjs files and secondly because even if it did, we need it to use a different config for different test suites. So that means we’re blocked on migrating because we can’t test any internal code that would depend on astro:i18n. This PR unblocks that.

I was planning to use this function for the container APIs, which will also accept a config.

It sounds like we may be somewhat overloading this one function? If it’s already used publicly for Vitest support, should we maybe not split it into some more dedicated helpers rather than also trying to use it for the container API?

In your case, I’d think we could refactor to a shared internal function that getViteConfig() and getContainerConfig() (made-up name) both use?

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Seems great to me, we do this a bunch already ourselves in our test utils, so feels fair that users would also want it. I don't have the context that Ema has, so happy to defer to him if there's something I'm missing here.

@ematipico
Copy link
Member

I checked the source code, and eventually, the configuration is merged and validated, so it's all good.

However, getViteConfigtries to load the configuration file already. I think you want to pass AstroInlineOnlyConfig instead of AstroInlineConfig, which accepts a configFile path.

@delucis
Copy link
Member Author

delucis commented May 7, 2024

I think you want to pass AstroInlineOnlyConfig instead of AstroInlineConfig, which accepts a configFile path.

Hmm, are you sure? AstroInlineConfig is what our JS APIs like dev() and build() support. AstroInlineOnlyConfig doesn’t support passing config options directly — you have to use a separate config file in this case.

@ematipico
Copy link
Member

I suppose I'll have to create a new function just for the container APIs then.

@ematipico ematipico merged commit 61f47a6 into main May 8, 2024
13 checks passed
@ematipico ematipico deleted the chris/get-vite-config branch May 8, 2024 10:19
@astrobot-houston astrobot-houston mentioned this pull request May 8, 2024
@delucis delucis added this to the 4.8.0 milestone May 8, 2024
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) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants