UnlyEd / next-right-now

Flexible production-grade boilerplate with Next.js 11, Vercel and TypeScript. Includes multiple opt-in presets using Storybook, Airtable, GraphQL, Analytics, CSS-in-JS, Monitoring, End-to-end testing, Internationalization, CI/CD and SaaS B2B multi single-tenancy (monorepo) support
https://unlyed.github.io/next-right-now/
MIT License
1.26k stars 111 forks source link

i18nextInstance prop is missing in types declarations in ssr/ssg pages #47

Closed 7iomka closed 4 years ago

7iomka commented 4 years ago

image This prop is provided for all pages through MultiversalAppBootstrapPageProps but not actually present in types SSGPageProps or SSRPageProps. Why? How to use it in pages without ts errors?

Vadorequest commented 4 years ago

That's a mistake on the TS typings then, I'll have a look at it.

7iomka commented 4 years ago

I tried to fix this with: pageProps.ts

export type SSGPageProps<E extends {} = {}> = {
  isStaticRendering: boolean;
} & MultiversalPageProps &
  Partial<MultiversalAppBootstrapPageProps> &
  E;
export type SSRPageProps<E extends OnlyServerPageProps = OnlyServerPageProps> = {
  isServerRendering: boolean;
} & MultiversalPageProps &
  Partial<MultiversalAppBootstrapPageProps> &
  E;

I thought that was enough, but this inconsistent. I mean to have access in autocompletion inside getCommonStaticProps or GetCommonServerSidePropsResults because it don't need properties from MultiversalAppBootstrapPageProps.

My hotFix:

  1. change StaticPropsOutput to receive generic
    export type StaticPropsOutput<Props = SSGPageProps> = {
    props: Props;
    unstable_revalidate?: number | boolean;
    };
  2. file ssg.ts
    
    export const getCommonStaticProps: GetStaticProps<
    Omit<SSGPageProps, keyof MultiversalAppBootstrapPageProps>,
    StaticParams
    > = async (
    props: StaticPropsInput,
    ): Promise<StaticPropsOutput<Omit<SSGPageProps, keyof MultiversalAppBootstrapPageProps>>> => {..}

3. file `ssr.ts`
```ts
export type GetCommonServerSidePropsResults = Omit<
  SSRPageProps,
  keyof MultiversalAppBootstrapPageProps | 'apolloState' | 'customer'
> & {
};

Can you suggest a better idea)

Vadorequest commented 4 years ago

Okay, first, why do you need to access i18nextInstance from the page? What do you want to do with it?

Vadorequest commented 4 years ago

@7iomka Fixed it with https://github.com/UnlyEd/next-right-now/pull/42/commits/58cd8a62319ac19b76c08774039b376501744721

Does that work for you?

Vadorequest commented 4 years ago

My bad, it hasn't fixed it, deploy has failed on vercel. I'll take a deeper look.

Vadorequest commented 4 years ago

@7iomka Don't hesitate to use @ts-ignore when running into that kind of time-wasting issues that don't actually affect the app. Most of the time, it's not worth spending too much time monkeypatching something like that! 😉

I believe https://github.com/UnlyEd/next-right-now/commit/b52802b07547780a8e10cb341c5605913b1460c7 should do it, I'll wait for CI to pass to confirm it.

Vadorequest commented 4 years ago

Indeed, fixed. Closing!

7iomka commented 4 years ago

@Vadorequest

  1. In your current implementation I don't see how to change locale programmatically without page refresh to get
    • new locale to be replaced in current browser url
    • new translation to be loaded from local source (json) if I'm not using locize I tied to do this for myself:
 const loadLang = React.useCallback(async (lang) => {
    if (!i18nextInstance.hasResourceBundle(lang, 'common')) {
      const i18nTranslations: I18nextResources = await getLocalTranslations(lang); // Load-translations from local JSON
      const newResource = i18nTranslations[lang].common || {};
      i18nextInstance.addResourceBundle(lang, 'common', newResource);
    }
    await i18nextInstance.changeLanguage(lang);
  }, []);
/**
 * Get JSON translations by language
 * @param lang language code string
 */
export const getLocaleJson = async (lang) => {
  return import(`../locales/${lang}.json`);
};

/**
 * Get JSON translations by current language
 * @param lang language code string
 * @return {Promise<string>}
 */
export const getLocalTranslations = async (lang: string): Promise<I18nextResources> => {
  let localeJson: I18nextResources = {};
  try {
    const jsonModule = await getLocaleJson(lang);
    localeJson = jsonModule.default || {};
    console.log({
      localeJson,
    });
  } catch (e) {
    logger.error(e.message, 'Failed to load JSON translations from local');
  }

  const i18nextResources: I18nextResources = {
    [lang]: {
      [defaultNamespace]: localeJson,
    },
  };

  logger.info(`Translations from local JSON was resolved ${getLocaleJsonPath(lang)}`);

  return i18nextResources;
};

This is just example (wip). I don't know, why you don't realize one source of truth how as done in next-i18next. This guys has realize class and provided the user with the opportunity to create an instance of the class with the settings they need. Then in any file of our app user can use customized components like Link, Router / useRouter (that already works with locale). As I see, you have an custom Link component, that works fine. But you not have cusomized Router, and, if you need to work with it api, like router.push you need also to include [locale], but I like to keep simple work with router like if locale is not included in url, to keep code in universal style. I don't know how to do that with your current implementation, because you manually hanlde [locale] in urls, for example in i18nRedirect https://github.com/UnlyEd/next-right-now/blob/f3c0487272245fc8bf4157be6c82f5f3b931e7d7/src/utils/app/router.ts#L87

2) I see you pass i18nextInstance from _app to be available on all pages, but it not used on the pages, nor available in autocompletion because wrong types. See my answer from above to fix all errors https://github.com/UnlyEd/next-right-now/issues/47#issuecomment-629348170

Vadorequest commented 4 years ago

You do have a Router, using Next.js router https://nextjs.org/docs/api-reference/next/router and that's all you need for programmatic usage.

i18nextInstance is provided to pages as a helper, but you most likely don't need it. What you need is the t function from react-i18next.

const { t, i18n } = useTranslation(); see https://react.i18next.com/latest/usetranslation-hook

The page now refreshes without full browser refresh, see demo at https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-dgkb8e7rn.now.sh/fr, it uses Next.js native router, as explained above.

Regarding your custom implementation using local translations instead of Locize, it's hard for me to help. (and too time-consumming)

7iomka commented 4 years ago

You do have a Router, using Next.js router https://nextjs.org/docs/api-reference/next/router and that's all you need for programmatic usage.

i18nextInstance is provided to pages as a helper, but you most likely don't need it. What you need is the t function from react-i18next.

const { t, i18n } = useTranslation(); see https://react.i18next.com/latest/usetranslation-hook

The page now refreshes without full browser refresh, see demo at https://nrn-v1-hyb-mst-aptd-gcms-lcz-sty-c1-dgkb8e7rn.now.sh/fr, it uses Next.js native router, as explained above.

Regarding your custom implementation using local translations instead of Locize, it's hard for me to help. (and too time-consumming)

Ok, but if I want to just simple go with router to another page or to add some query parameters, I need everytime to make some url calculation like this?:

const router = useRouter();
router.push('/products'); // that won't work!
router.push(`/${locale}/products`); // that maybe will work, but only in code that have access to actual locale
Vadorequest commented 4 years ago

If you need to do it programmatically, yes. Can't you use I18nLink component instead?

router.push(/${locale}/products); won't work, you need to handle query parameters properly (it's how Next works), so it'd rather be: router.push(/[locale]/products,/${locale}/products);

And yes, in such case you'll need to know locale, but you can access it from any component using hooks by doing const { locale }: I18n = useI18n();

7iomka commented 4 years ago

@Vadorequest In next.js dynamic routes are written

router.push('/somepage/[id]', `/somepage/${myId}`)

since we have [locale] - a required part of any router.pathname this creates certain inconveniences associated with the fact that we ALWAYS need to include the second parameter in router.push even if the route does not contain other dynamic parts, since [locale] is automatically in any of them that is, to simply go to the product page, we need

router.push(/[locale]/products, /${locale}/products)

What would a wrapper do over a next router? would allow us to exclude this case and work with the router as if we don’t have a dynamic locale parameter

what i write ===> what is actually happens thanks for our wrapper

router.push('/products') ==> router.push('/en/products')
router.push('/products/[id]', `/products/${productId}`) ==> router.psh('/en/products/[id]', `/products/${productId}`)

Exist any idea how easy to make this kind of wrapper over next router to to achieve this behavior? Thanks!

I tried:

const propertyFields = ['pathname', 'route', 'query', 'asPath', 'components', 'events'];
const coreMethods = ['reload', 'back', 'beforePopState', 'ready', 'prefetch'];
const wrappedMethods = ['push', 'replace'];

export const Router = ((locale) => {
  const R = {};
  propertyFields.forEach((field) => {
    Object.defineProperty(R, field, {
      get() {
        return nextRouter[field];
      },
    });
  });
  coreMethods.forEach((method) => {
    R[method] = (...args) => nextRouter[method](...args);
  });
  wrappedMethods.forEach((method) => {
    R[method] = (href, as, options) => {
        const { i18nHref, i18nAs }: I18nRoute = resolveI18nRoute({ href, as, locale });
        return nextRouter[method](i18nHref, i18nAs, options);
    };
  });
  return R;
})('en');

BUT, this code I write in utils/app/router an we not have access to actual active language, and when it changes, we don't see any updates. And I don’t know what to do better and where to do it. Thanks for any ideas.

Vadorequest commented 4 years ago

Yup, it's similar to what I've done already with I18nLink component.

We aren't supposed to use the router dynamically that much though, the only use case I've got so far is when switching languages. Building a helper or wrapper that does this is possible, but I think i18nRedirect already answers your need.

/**
 * Redirects the current page to the "same" page, but for a different locale
 *
 * @param locale
 * @param router
 * @param pageReload
 * @see https://nextjs.org/docs/routing/imperatively Programmatic usage of Next Router
 * @see https://nextjs.org/docs/api-reference/next/router#router-api Router API
 */
export const i18nRedirect = (locale, router: NextRouter, pageReload = false): void => {
  const newUrl = `${router.pathname.replace('[locale]', locale)}`;

  if (pageReload) {
    location.href = newUrl;
  } else {
    router.push(router.pathname, newUrl);
  }
};

What it misses is another parameter for the newUrl, it currently uses a hardcoded default, but it could by dynamic. Would that solve your issue? I believe so.

7iomka commented 4 years ago

@Vadorequest , router has many methods that user can use in production - replace, prefetch... Without common wrapper how to do that? no way. i18nRedirect just do what named - redirect to provided locale. Basic goal to make an common wrapper - to use whole next router api without worrying about [locale] and do not write unnecessary boilerplate code to just include [locale] in each call even for router that not have another dynamic parts.

Vadorequest commented 4 years ago

I understand, but I still don't believe you need something so complicated.

If your only use case is to use router.push, then writing a helper will do the job.

If you want to support a more complicated setup, then extending the native Next router makes more sense, as you pointed out. (but I don't see the use-case for this)

If you really want to write a wrapper, you should do it in /components, not in /utils. Because you'll want a React component that can access the router value. Similarly to what I did with I18nLink component.

7iomka commented 4 years ago

const { t, i18n } = useTranslation(); see https://react.i18next.com/latest/usetranslation-hook

if I access to i18n instance it is not the same as i18nextInstance from the props. Why?

Vadorequest commented 4 years ago

i18nextInstance is not meant to be used, it's just provided "in-case-of", but what's meant to be used is useTranslation() hook.