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

Fix 1181: Missing namespaces if they are not in the defaultLanguage (but exist in the fallbackLng) #1197

Merged
merged 10 commits into from
May 21, 2021

Conversation

Nikoms
Copy link
Contributor

@Nikoms Nikoms commented May 13, 2021

Fix for #1181

Tell me if I need to modify things 🤓

In createConfig.test.ts, I had to add a as unknown for fallbackLng (line 64). Don't know why, because it's a valid value.

Cheers!

@vercel
Copy link

vercel bot commented May 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/isaachinman/next-i18next/4Q5Q3ff2bBG7GgEgxxzcksomhzFE
✅ Preview: https://next-i18next-git-fork-nikoms-fix-1181-isaachinman.vercel.app

@Nikoms
Copy link
Contributor Author

Nikoms commented May 13, 2021

I'm working on the code to make it pass on codeclimate and multi-build 😁

I don't know if all remarks of codeclimate are justified, (Similar blocks for expected variable in tests) but I'll do my best :)

@Nikoms
Copy link
Contributor Author

Nikoms commented May 13, 2021

Done 😘

})

it('gets namespaces from current language + fallback (as object) when ns is not provided', ()=>{
const fallbackLng = {default: ['fr'], 'en-US': ['en']} as unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const fallbackLng = {default: ['fr'], 'en-US': ['en']} as unknown
const fallbackLng = { default: ['fr'], 'en-US': ['en'] } as unknown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that these rules could be added in the linter?

)
combinedConfig.ns = getAllNamespaces(path.resolve(process.cwd(), `${serverLocalePath}/${lng}`))
if (!combinedConfig.ns && typeof lng !== 'undefined') {
const unique = (list:string[]) => Array.from(new Set<string>(list))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const unique = (list:string[]) => Array.from(new Set<string>(list))
const unique = (list: string[]) => Array.from(new Set<string>(list))

Comment on lines 89 to 93
.reduce(
(flattenNamespaces, namespaces) =>
[...flattenNamespaces,...namespaces],
[]
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to achieve this without using reduce?

Comment on lines 102 to 107
if (typeof fallbackLng === 'string')
return unique([lng, fallbackLng])

if (Array.isArray(fallbackLng))
return unique([lng, ...fallbackLng])

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use curly braces for any if statements.

if (typeof fallbackLng === 'object') {
const flattenedFallbacks = Object
.values(fallbackLng)
.reduce(((all, fallbackLngs) => [...all,...fallbackLngs]),[])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.reduce(((all, fallbackLngs) => [...all,...fallbackLngs]),[])
.reduce(((all, fallbackLngs) => [ ...all, ...fallbackLngs ]),[])

const flattenedFallbacks = Object
.values(fallbackLng)
.reduce(((all, fallbackLngs) => [...all,...fallbackLngs]),[])
return unique([lng, ...flattenedFallbacks])
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return unique([lng, ...flattenedFallbacks])
return unique([ lng, ...flattenedFallbacks ])


const DEFAULT_CONFIG_PATH = './next-i18next.config.js'

const getFallBackLocales = (fallbackLng: false | FallbackLng) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const getFallBackLocales = (fallbackLng: false | FallbackLng) => {
const getFallbackLocales = (fallbackLng: false | FallbackLng) => {

if (typeof fallbackLng === 'object') {
return Object
.values(fallbackLng)
.reduce((all, locales) => [...all, ...locales],[])
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, can we use a different method than reduce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a loop. There is array.flat() but it's not available in node 10 :(

if (Array.isArray(fallbackLng)) {
return fallbackLng
}
if (typeof fallbackLng === 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this condition also enter for null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but I wonder if the fallbackLng can have the null value. I'll add an extra condition :)

Comment on lines 81 to 83
.reduce(
(allNamespaces, namespaces) => [...allNamespaces,...namespaces],
[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, possible to use something other than reduce?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course :)

@Nikoms
Copy link
Contributor Author

Nikoms commented May 14, 2021

Done everything 🥳 Cyclomatic complexity to 5 is a challenge 😁

@Nikoms
Copy link
Contributor Author

Nikoms commented May 21, 2021

Hi @isaachinman , do I have to do something more for this PR?

@isaachinman
Copy link
Contributor

Nope, looks reasonable to me! Thanks for all your hard work @Nikoms!

@isaachinman isaachinman merged commit 7474c93 into i18next:master May 21, 2021
@Nikoms
Copy link
Contributor Author

Nikoms commented May 22, 2021

Thank youuu, I'll wait for the "official" new version. I believe 8.3.1 ?

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.

2 participants