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

Simplifies hasValidReactChildren #1764

Merged
merged 1 commit into from
Jul 15, 2024

Conversation

VIKTORVAV99
Copy link
Contributor

Simplifies hasValidReactChildren a bit by utilizing Array.isArray, implicit returns and the fact that .every() can take a function without arguments as it's own argument.

This no longer returns false when it's not an array but rather implicitly returns undefined which don't affect current functionality as the code it's used in just checks for truthy values.

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

@coveralls
Copy link

Coverage Status

coverage: 97.143% (-0.003%) from 97.146%
when pulling f97b826 on VIKTORVAV99:simplify_hasValidReactChildren
into a904e07 on i18next:master.

@adrai adrai merged commit 92bc147 into i18next:master Jul 15, 2024
8 of 9 checks passed
@VIKTORVAV99
Copy link
Contributor Author

VIKTORVAV99 commented Jul 15, 2024

@adrai how do you feel about making some changes that might potentially be breaking changes?

I'm thinking about optional chaining and removing the code here:

// WAIT A LITTLE FOR I18NEXT BEING UPDATED IN THE WILD, before removing this old i18next version support
const oldI18nextHasLoadedNamespace = (ns, i18n, options = {}) => {
const lng = i18n.languages[0];
const fallbackLng = i18n.options ? i18n.options.fallbackLng : false;
const lastLng = i18n.languages[i18n.languages.length - 1];
// we're in cimode so this shall pass
if (lng.toLowerCase() === 'cimode') return true;
const loadNotPending = (l, n) => {
const loadState = i18n.services.backendConnector.state[`${l}|${n}`];
return loadState === -1 || loadState === 2;
};
// bound to trigger on event languageChanging
// so set ready to false while we are changing the language
// and namespace pending (depends on having a backend)
if (
options.bindI18n &&
options.bindI18n.indexOf('languageChanging') > -1 &&
i18n.services.backendConnector.backend &&
i18n.isLanguageChangingTo &&
!loadNotPending(i18n.isLanguageChangingTo, ns)
)
return false;
// loaded -> SUCCESS
if (i18n.hasResourceBundle(lng, ns)) return true;
// were not loading at all -> SEMI SUCCESS
if (
!i18n.services.backendConnector.backend ||
(i18n.options.resources && !i18n.options.partialBundledLanguages)
)
return true;
// failed loading ns - but at least fallback is not pending -> SEMI SUCCESS
if (loadNotPending(lng, ns) && (!fallbackLng || loadNotPending(lastLng, ns))) return true;
return false;
};

Both of these changes should be supported by the current build config as far as I can tell. Browserslist is configured to only target the default browsers and the code in question is never used if the peer dependency version of i18next is followed.

@adrai
Copy link
Member

adrai commented Jul 15, 2024

same topic like i18next/i18next#2184

@VIKTORVAV99
Copy link
Contributor Author

What about removing the old code then? That has nothing to do with system compatibility and should be fine as long as the peer versions are followed.

@adrai
Copy link
Member

adrai commented Jul 15, 2024

yes, I think 3 years later, we can remove that old code for a future major version (also conditional chaining)...
as soon as you "finished" to create other PRs... (non-breaking) I will create a NON-major version, and afterwards we can go for a new major version, ok?

@VIKTORVAV99
Copy link
Contributor Author

Sounds good!

I'll go through the code a final time and see if I find any other small opportunities to optimise the current code without breaking changes.

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.

3 participants