QuiiBz / next-international

Type-safe internationalization (i18n) for Next.js
https://next-international.vercel.app
MIT License
1.25k stars 59 forks source link

[Code Safety][Suggestion] Type more strictly ImportedLocales #221

Closed gustaveWPM closed 10 months ago

gustaveWPM commented 11 months ago

At the moment, ImportedLocales is typed as follows:

type ImportedLocales = Record<string, () => Promise<any>>;

I find this troublesome because the leaf nodes of the vocabulary objects should be ALWAYS strings (unless I've missed something?).

We could perhaps create a LocaleData type that forces the object's leaf nodes to be only strings.

type LocaleValue = string; // * ... sum type

type LocaleData = {
  [_: string]: LocaleData | LocaleValue;
};

EDIT: actually, LocaleValue already exists in international-types. But I don't understand why it could be also a number, or a boolean, or null or undefined? :/ Is it to be 100% compatible with JSON standards? If so, undefined is inappropriate. And I think that undefined is inappropriate in any case. However, I admit that I didn't think about Date, which is highly desirable.

export type LocaleValue = string | number | boolean | null | undefined | Date; // * ... LocaleValue, as defined in international-types

So, we could do:

type ImportedLocales = Record<string, () => Promise<LocaleData>>;

Maybe like this? (I think this code snippet illustrates my idea quite well.)

type LocaleData = {
  [_: string]: LocaleData | LocaleValue;
};

const localesToImport = {
  en: () => import('@/i18n/locales/en')
};

function doImports(importedLocales: Record<string, () => Promise<LocaleData>>) {
  // * ...
}

doImports(localesToImport);

I find it really unfortunate that currently, it is possible to write things like this without encountering any typing errors at build:

// locales/en.ts

export default {
  whatever: {
    valid: 'en', // * ... Is a string
    invalid: 12 // * ... Is a number
  }
} as const;
// client.ts

import { createI18nClient } from 'next-international/client';

export const { useI18n: getClientSideI18n, useScopedI18n, I18nProviderClient, useCurrentLocale } = createI18nClient({
  en: () => import('@/i18n/locales/en'),
});
// server.ts

import { createI18nServer } from 'next-international/server';

export const { getI18n: getServerSideI18n, getScopedI18n, getStaticParams } = createI18nServer({
  en: () => import('@/i18n/locales/en'),
});
gustaveWPM commented 11 months ago

Just to give an idea of how I deal with it atm:

type JSONPrimitiveLeafs = string | number | boolean | null;
type JSONLeafs = JSONPrimitiveLeafs | JSONPrimitiveLeafs[];

export type JSONData = {
  [_: string]: JSONData | JSONData[] | JSONLeafs;
};

export type TypedLeafsJSONData<LeafsTypes extends JSONLeafs, AllowObjArrays extends 'ALLOW_OBJ_ARRAYS' = never> = {
  [_: string]: TypedLeafsJSONData<LeafsTypes> | (AllowObjArrays extends never ? never : TypedLeafsJSONData<LeafsTypes>[]) | LeafsTypes;
};
export enum ELanguagesFlag {
  fr,
  en
}
type LanguageFlagKey = keyof typeof ELanguagesFlag;
export type LanguageFlag = LanguageFlagKey;
export type LocalesGetterConfigObjTypeConstraint = Record<LanguageFlag, () => Promise<TypedLeafsJSONData<string>>>;
import { LocalesGetterConfigObjTypeConstraint } from '@/types/i18n';

const getLocales = () =>
  ({
    fr: () => import('@/i18n/locales/fr'),
    en: () => import('@/i18n/locales/en')
  } satisfies LocalesGetterConfigObjTypeConstraint);

export const locales = getLocales();
export default locales;
import { createI18nClient } from 'next-international/client';
import { locales } from './getLocales';

export const { useI18n: getClientSideI18n, useScopedI18n, I18nProviderClient, useCurrentLocale } = createI18nClient(locales);
import { createI18nServer } from 'next-international/server';
import { locales } from './getLocales';

export const { getI18n: getServerSideI18n, getScopedI18n, getStaticParams } = createI18nServer(locales);
QuiiBz commented 10 months ago

I just tried restricting ImportedLocales to be Record<string, () => Promise<{ default: Record<string, unknown> }>>, and while it can help a bit, it still allows the import any module that has a default export being an object. I can even import a next.config.js file and it will work, which makes sense because it matches the signature. It would only restrict importing non-js/ts files, or files not exporting a default object (which again, can be biased).

Because of this, I don't think going further in this direction will really improve the DX. It's already pretty clear from the documentation and examples that the locales file must match this specific signature.

Quick note: LocaleValue isn't meant to be used as the value of locales object, these should only be strings. Instead, it should only be used as the possible values of parameters when translating.

gustaveWPM commented 10 months ago

I just tried restricting ImportedLocales to be Record<string, () => Promise<{ default: Record<string, unknown> }>>, and while it can help a bit, it still allows the import any module that has a default export being an object. I can even import a next.config.js file and it will work, which makes sense because it matches the signature. It would only restrict importing non-js/ts files, or files not exporting a default object (which again, can be biased).

Because of this, I don't think going further in this direction will really improve the DX. It's already pretty clear from the documentation and examples that the locales file must match this specific signature.

Quick note: LocaleValue isn't meant to be used as the value of locales object, these should only be strings. Instead, it should only be used as the possible values of parameters when translating.

Oh, okay. I'm sorry, I didn't quite anticipated some subtleties, and in the end my solution was really poor. Even if we decided to force all the object's leaves to be strings (instead of Record<string, unknown>), this would still be very ambiguous.

I was wrong!

QuiiBz commented 10 months ago

Nothing wrong with being wrong! It's still interesting to understand the specifics and the why/how. Thanks for the suggestion!