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 performance issue that affects flat object with multiple (thousands) keys #2004

Merged
merged 1 commit into from Jul 26, 2023

Conversation

pedrodurek
Copy link
Member

@pedrodurek pedrodurek commented Jul 26, 2023

Closes #1991

In this PR, I've simplified the approach for inferring the return type by directly returning the value of a specified key, rather than using conditional type to verify the presence of all KeyWithOrdinalPlural, KeyWithPlural, and Key within Res before returning the value.

Here are the complexity results for the old and new approaches:

Old approach

  • N = Number of sub-keys in the same key or namespace
  • KeyWithPlural = 6
  • KeyWithOrdinalPlural = 6

Complexity: O((KeyWithPlural * N) + (KeyWithOrdinalPlural * N) + N) = O(13 * N)

New approach

Complexity: O(1) - constant time

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

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

@coveralls
Copy link

coveralls commented Jul 26, 2023

Coverage Status

coverage: 92.36%. remained the same when pulling df6c31c on pedrodurek:fix-performance-issue into 815cd83 on i18next:master.

@adrai adrai merged commit 8fa62ef into i18next:master Jul 26, 2023
5 checks passed
@adrai
Copy link
Member

adrai commented Jul 26, 2023

included in v23.3.0

@quimeyruffa
Copy link

Hi! I got this error in one of the files of the node modules, do you know why? Has anyone else experienced this?

[0m[91m./node_modules/i18next/index.d.ts:834:69
Type error: '?' expected.

@adrai
Copy link
Member

adrai commented Jul 28, 2023

@quimeyruffa Which ts version are you using? And please provide a repruducible example.

@quimeyruffa
Copy link

quimeyruffa commented Jul 28, 2023

@adrai
I am using version v23.3.0, and this is my i18n configuration.

import i18n from 'i18next';
import { initReactI18next } from 'react-i18next';
import es from './locales/AR.json'

const resources = {
    es: {
        translation: es
    }
}

i18n.use(initReactI18next).init({
    resources,
    defaultNS: "translation",
    fallbackLng: "es-AR",
    lng: "es-AR",
    debug: false,
  })

export const loadLanguage = (locale?: string) => {
    if (!locale) return

    import(`./locales/${locale}.json`).then((resources) => {
        i18n.addResourceBundle(locale, "translation", resources)
        i18n.changeLanguage(locale)
    })
}

@adrai
Copy link
Member

adrai commented Jul 28, 2023

@quimeyruffa I mean the TypeScript version... and btw: that's not a reproducible example... please provide a reproducible example like a github repo or similar.

@quimeyruffa
Copy link

Oh, sorry, I misunderstood, the version is. "typescript": "4.3.5"
The code itself cannot be sent due to company policy, but the error itself occurs at build-time and is related to the loadLanguage() function.
Here is the package.json.

{
  "name": "pharma1",
  "version": "0.1.0",
  "private": true,
  "scripts": {
    "dev": "NODE_OPTIONS='--openssl-legacy-provider' next dev -p 3008",
    "build": "next build",
    "start": "next start",
    "cy:open": "cypress open",
    "cy:run": "cypress run",
    "cy:install": "cypress install",
    "test:unit": "NODE_ENV=test jest --silent --env=jsdom",
    "test:unit:verbose": "NODE_ENV=test jest --verbose --env=jsdom",
    "test:unit:debug": "DEBUG=* npm run test:unit --silent=false",
    "test:e2e": "NODE_ENV=test npm run cy:run",
    "test:watch": "jest --watchAll",
    "test:coverage": "jest --watch=false --coverage"
  },
  "dependencies": {
    "@material-ui/core": "4.11.0",
    "@material-ui/icons": "4.9.1",
    "@material-ui/lab": "4.0.0-alpha.57",
    "@material-ui/styles": "4.10.0",
    "@xstate/react": "1.5.1",
    "autosuggest-highlight": "3.1.1",
    "axios": "0.21.1",
    "classnames": "^2.3.1",
    "compression": "^1.7.4",
    "cors": "^2.8.5",
    "date-fns": "2.15.0",
    "express": "^4.18.2",
    "faker": "5.1.0",
    "framer-motion": "4.1.17",
    "i18next": "^23.3.0",
    "js-cookie": "3.0.0",
    "msw": "0.34.0",
    "next": "11.1.0",
    "polished": "4.1.3",
    "pusher-js": "7.0.2",
    "rc-pagination": "3.0.3",
    "react": "17.0.1",
    "react-dom": "17.0.1",
    "react-dropzone": "11.2.3",
    "react-error-boundary": "3.0.2",
    "react-google-recaptcha": "2.1.0",
    "react-hook-form": "6.4.1",
    "react-i18next": "^13.0.2",
    "react-perfect-scrollbar": "1.5.8",
    "styled-components": "5.1.1",
    "styled-system": "5.1.5",
    "swr": "0.5.6",
    "typeface-source-sans-pro": "1.1.5",
    "xstate": "4.12.0"
  },
  "devDependencies": {
    "@next/bundle-analyzer": "11.1.0",
    "@testing-library/cypress": "8.0.0",
    "@testing-library/dom": "8.1.0",
    "@testing-library/jest-dom": "5.11.4",
    "@testing-library/react": "12.0.0",
    "@testing-library/user-event": "13.2.1",
    "@types/faker": "5.1.2",
    "@types/jest": "27.0.0",
    "@types/node": "16.6.0",
    "@types/react": "17.0.17",
    "@types/styled-components": "5.1.4",
    "@types/styled-system": "5.1.10",
    "babel-jest": "27.0.6",
    "babel-plugin-styled-components": "1.11.1",
    "cypress": "8.2.0",
    "husky": "7.0.1",
    "jest": "27.0.6",
    "jest-dom": "4.0.0",
    "prettier": "2.1.2",
    "typescript": "4.3.5"
  }
}

@adrai
Copy link
Member

adrai commented Jul 28, 2023

sorry, I'm unable to help without a reproducible example, but if the error occurs in loadLanguage, your build setup might be wrong...

@quimeyruffa
Copy link

this is the build log

info  - Checking validity of types...
Failed to compile.

./node_modules/i18next/index.d.ts:834:69
Type error: '?' expected.

  832 | type ParseKeysByFallbackNs<Keys extends $Dictionary> = _FallbackNamespace extends false
  833 |   ? never
> 834 |   : _FallbackNamespace extends (infer UnionFallbackNs extends string)[]
      |                                                                     ^
  835 |   ? Keys[UnionFallbackNs]
  836 |   : Keys[_FallbackNamespace & string];
  837 | 
error Command failed with exit code 1.

@pedrodurek
Copy link
Member Author

This syntax is only supported in typescript V4.5 or higher, I'll put a PR up to change the syntax here.

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.

Worse typechecking performance with version 23
4 participants