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

[WIP] Support for fallbackLng (substituable language) #66

Merged
merged 5 commits into from
Apr 25, 2023

Conversation

michalpulpan
Copy link
Contributor

@michalpulpan michalpulpan commented Mar 9, 2023

This PR tries to implement logic supporting fallbackLng object in the i18n config (more described in the #58).
The requirement was basically to implement:

{
   locales: ['fr', 'fr-FR', 'fr-BE', 'en', 'es', 'pt'],
   fallbackLng: {
    'fr-FR': ['fr'],
    'fr-BE': ['fr'],
   }
}

So that we don't have to store redundant data in our _routes.json and be fully compactible with i18n-next fallbackLng.

It implements a function getPathFromPaths (sorry for this naming getPath is already used plugin/getRouteBranchReRoutes.ts 👀 ).
It was necessary to extend types.ts a bit with (fallbackLng object in TNtrData) and to create custom NTRI18NConfig extending original I18NConfig.

With this we should be now able to resolve the logic described above.

❗ But, this doesn't (yet) implement logic of nonExplicitSupportedLngs and it could still raise warning of Max custom routes reached. I'm currently not 100% sure how to implement it so we could do different PR for it?.

All previous test cases pass and since we have to use different defaultNtrData in test cases (due to fallbackLng dict
) I added setEnvDataFallbackLng function with which we have to run before fallbackLng is tested (or it's presence). Hope it's clear and okay like this. It's a lot of redundant test but I did not workout how to make basically test it more intelligently without ruining previous tests (and adding fallbackLng everywhere).

Thank you for your feedback @cvolant !

Copy link
Collaborator

@cvolant cvolant left a comment

Choose a reason for hiding this comment

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

Hi @michalpulpan, thank you for your PR.

It sounds good to me.
Concerning the redundant tests, you can leave it like this. One day perhaps, I may merge them with the others.

There is only 2 little things: see comments below.
Oh, and I don't understand why the yarn.lock has changed. Maybe we don't have the same node version or something.

If you want to implement nonExplicitSupportedLngs, it is probably better to do it in a new PR.

This PR is currently a draft: once you are ready, you can launch the example locally, then check that everything works. It it does, you can leave a comment to say it does, remove the "draft" state, then ask for a final review. I will then check it too, then merge it.

src/plugin/getPathFromPaths.ts Show resolved Hide resolved
tests/react/setEnvData.ts Outdated Show resolved Hide resolved
@cvolant
Copy link
Collaborator

cvolant commented Apr 11, 2023

Hi @michalpulpan! Any news on this?

@michalpulpan
Copy link
Contributor Author

michalpulpan commented Apr 11, 2023

Hi @michalpulpan! Any news on this?

Hi, I'm sorry, I completely forgot about this 🤦 . I'll do it ASAP!

Oh, and I don't understand why the yarn.lock has changed. Maybe we don't have the same node version or something.

I din't notice it. Do you think I should restore it to the version in a main branch? 🤔

@michalpulpan
Copy link
Contributor Author

@cvolant Pushed proposed fixes. 😊 Please let me know how to deal with the yarn.lock. Otherwise I'm ready!

@cvolant cvolant marked this pull request as ready for review April 25, 2023 10:36
@cvolant
Copy link
Collaborator

cvolant commented Apr 25, 2023

Hi @michalpulpan, thank you for the update, and sorry for the delay, I was off for some days.

To solve the yarn.lock, you can try: git checkout master -- yarn.lock.

Then I will merge it and release a new version.

@michalpulpan
Copy link
Contributor Author

Hi @michalpulpan, thank you for the update, and sorry for the delay, I was off for some days.

To solve the yarn.lock, you can try: git checkout master -- yarn.lock.

Then I will merge it and release a new version.

Hi, no problem 😊. Thank you for the trick, hopefully it worked out and yarn.lock is merged from master

@cvolant cvolant merged commit 5d5085c into hozana:master Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants