refactor: unsplit astro:i18n module
#9790
Conversation
🦋 Changeset detectedLatest commit: 70d5a85 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
the
pkg: astro
| if (i18n === undefined) throw new Error("The `astro:i18n` module can not be used without enabling i18n in your Astro config."); | ||
| return this.resolve("astro/virtual-modules/i18n.js"); |
There was a problem hiding this comment.
The error here is new. Previously, it would throw with a generic "cant resolve" message.
There was a problem hiding this comment.
Should we throw an AstroError instead? Or use this.error?
There was a problem hiding this comment.
I am not sure if one of them is better. I can say this will properly display the message in the browser error overlay.
There was a problem hiding this comment.
I think this should be an AstroError, yes! I didn't fully understand the this.error thing though
There was a problem hiding this comment.
I think this should be an AstroError, yes! I didn't fully understand the this.error thing though
In a rollup plugin you can use this.error to throw an error without breaking too much stuff :D
There was a problem hiding this comment.
this.error()
throw new Error()
throw new AstroError()
There was a problem hiding this comment.
I'd go with a AstroError so that we can have docs and stuff on it then!
packages/astro/src/i18n/index.ts
Outdated
| return code; | ||
| } | ||
| } else if (locale === path) { | ||
| return locale; | ||
| } | ||
| } | ||
| return undefined; | ||
| throw new Error("Unreachable"); |
There was a problem hiding this comment.
Added errors in a couple of places where implementation did not match the public API.
There was a problem hiding this comment.
I suggest inserting a better message here. Even though it's for us, it's good practice to leave a message explaining why this happened, or a possible fix.
| import type { I18nInternalConfig } from "../i18n/vite-plugin-i18n.js"; | ||
| export { normalizeTheLocale, toCodes, toPaths } from "../i18n/index.js"; | ||
| // @ts-expect-error | ||
| import config from "astro-internal:i18n-config" |
There was a problem hiding this comment.
Something that I didn't know about virtual modules an vite. Nice!
| const { defaultLocale, locales, routing, fallback } = i18n!; | ||
| const config: I18nInternalConfig = { base, format, site, trailingSlash, defaultLocale, locales, routing, fallback }; | ||
| return `export default ${JSON.stringify(config)};`; |
There was a problem hiding this comment.
Definitely more maintainable!
There was a problem hiding this comment.
yep, and this will be the key to removing i18n from the manifest
| if (i18n === undefined) throw new Error("The `astro:i18n` module can not be used without enabling i18n in your Astro config."); | ||
| return this.resolve("astro/virtual-modules/i18n.js"); |
There was a problem hiding this comment.
Should we throw an AstroError instead? Or use this.error?
packages/astro/src/i18n/index.ts
Outdated
| return code; | ||
| } | ||
| } else if (locale === path) { | ||
| return locale; | ||
| } | ||
| } | ||
| return undefined; | ||
| throw new Error("Unreachable"); |
There was a problem hiding this comment.
I suggest inserting a better message here. Even though it's for us, it's good practice to leave a message explaining why this happened, or a possible fix.
Changes
astro:i18nmodule lived inastro/client.vite-plugin-i18n.ts.astro/virtual-modules/i18n.tsTesting
Does not affect behavior. Existing tests should pass.
Docs
Does not affect usage.