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

Issues with i18n config typing #10157

Closed
1 task
delucis opened this issue Feb 19, 2024 · 6 comments · Fixed by #10169
Closed
1 task

Issues with i18n config typing #10157

delucis opened this issue Feb 19, 2024 · 6 comments · Fixed by #10169
Assignees
Labels
needs triage Issue needs to be triaged

Comments

@delucis
Copy link
Member

delucis commented Feb 19, 2024

Astro Info

Astro                    v4.4.0
Node                     v18.18.0
System                   Linux (x64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

Any (n/a)

Describe the Bug

Using this issue to report two related issues with the types for the i18n config.

  1. The types for i18n.routing are inaccurately strict. For example, the following is valid configuration and shown in the Astro docs:

    import { defineConfig } from 'astro/config';
    
    export default defineConfig({
      i18n: {
        defaultLocale: 'en',
        locales: ['en', 'fr'],
        routing: {
          prefixDefaultLocale: true,
        },
      },
    });

    However, the above is considered a type error (you need an astro.config.ts or a // @ts-check comment to see this most likely). strategy and redirectToDefaultLocale are both typed as required in i18n.routing.

  2. When setting i18n configuration from an Astro integration via the updateConfig() method, it seems to expect one of the internal enum values like 'pathname-prefix-other-locales' rather than the proper configuration object. I suspect the issue is that updateConfig() is typed with AstroConfig instead of AstroUserConfig, which I think is inaccurate?

What's the expected result?

  1. The types should not require i18n.routing be set with all values.

  2. The updateConfig() integration method should accept the same shape as when setting the config directly.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-cosd7e?file=astro.config.ts

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Feb 19, 2024
@matthewp matthewp added - P4: important Violate documented behavior or significantly impacts performance (priority) and removed needs triage Issue needs to be triaged labels Feb 19, 2024
@matthewp matthewp self-assigned this Feb 19, 2024
@matthewp matthewp added needs triage Issue needs to be triaged and removed - P4: important Violate documented behavior or significantly impacts performance (priority) labels Feb 19, 2024
@matthewp
Copy link
Contributor

Moved this back to "needs triage" after looking at the code. I'm not completely certain that this is a bug or not. The code all uses AstroConfig rather than AstroUserConfig. And you do get the zod-parsed config during setup. So I think the expectation is that integrations will mutate the resolved config, rather than user-defined config.

@delucis
Copy link
Member Author

delucis commented Feb 19, 2024

you do get the zod-parsed config during setup. So I think the expectation is that integrations will mutate the resolved config, rather than user-defined config.

Do we have other parts of the config that do radical transformations like i18n.routing? Feels like usually we might apply a default at most. Might make sense to keep the config shape consistent and move any mapping to the string form into the feature itself if possible.

@matthewp
Copy link
Contributor

Yeah, normally we don't do dramatic changes. State derived from config usually goes on the AstroSettings object. @ematipico would it be possible to move this derived state there and to keep the config structure intact?

@ematipico
Copy link
Member

I was under the impression that user config and integrations config is the same.

If not, they should. The transformation that I applied was supposed to be internal to Astro.

What's the best course of action?

@delucis
Copy link
Member Author

delucis commented Feb 20, 2024

What's the best course of action?

I’m not super familiar with how the i18n code uses the config, but Matthew suggested above that AstroSettings could get the transformed form. In other words this mapping of the config to the string form would happen there?

.transform((i18n) => {
if (i18n) {
let { routing, domains } = i18n;
let strategy: RoutingStrategies;
const hasDomains = domains ? Object.keys(domains).length > 0 : false;
if (!hasDomains) {
if (routing.prefixDefaultLocale === true) {
if (routing.redirectToDefaultLocale) {
strategy = 'pathname-prefix-always';
} else {
strategy = 'pathname-prefix-always-no-redirect';
}
} else {
strategy = 'pathname-prefix-other-locales';
}
} else {
if (routing.prefixDefaultLocale === true) {
if (routing.redirectToDefaultLocale) {
strategy = 'domains-prefix-always';
} else {
strategy = 'domains-prefix-always-no-redirect';
}
} else {
strategy = 'domains-prefix-other-locales';
}
}
return { ...i18n, routing: strategy };
}
return undefined;
})

Would i18n code be able to read that off AstroSettings? (Or be refactored to skip this transformation and check the config object directly?)

@ematipico
Copy link
Member

I mean, everything is possible, but I believe that not doing transformations as we want is a huge constraint, and this issue shows it clearly.

I'll have to think about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage Issue needs to be triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants