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

Make typescript autocomplete aware of contextSeparator #1334

Conversation

JCQuintas
Copy link
Contributor

This makes the react-i18next types aware of the contextSeparator, we might want to make it possible to change it later, similar to how we allow changing the defaultNS, but for now it should be simple to fix and user's issue.

How it works: keys that end in _${string} will be added with and without the ending to the type auto-completion.

eg:

const resources = {
  translation: {
    type_male: "Male",
    type_female: "Female",
  }
}

t('type') // valid
t('type_male') // valid
t('type_female') // valid
t('type_missing') // invalid

fixes #1333

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • documentation is changed or added

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.583% when pulling 00eb74a on JCQuintas:feature/context-separator-typescript-autocomplete into 4ae8a84 on i18next:master.

@pedrodurek
Copy link
Member

pedrodurek commented Jun 16, 2021

Thanks @JCQuintas for that! I may take a few days to review it because I'll need to measure and test how much in terms of time complexity has increased. We're recursively mapping all keys & return types and any extra complexity might affect compilation time (big applications mostly).

@stale
Copy link

stale bot commented Jun 24, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 24, 2021
@pedrodurek pedrodurek removed the stale label Jun 24, 2021
@stale
Copy link

stale bot commented Jul 1, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 1, 2021
@pedrodurek pedrodurek removed the stale label Jul 5, 2021
@JCQuintas
Copy link
Contributor Author

Is there any way I can help testing this out?

@pedrodurek
Copy link
Member

Hey @JCQuintas, sorry about the delay, these last couple of weeks have been challenging for me, I'll finish reviewing it today!

@JCQuintas
Copy link
Contributor Author

@pedrodurek no worries. I'm in no hurry 😉

@pedrodurek
Copy link
Member

Hey @JCQuintas, I finally finished all tests, but unfortunately, I don't have great news. Compilation time got ~10x slower than before (retrieve keys and return type) with the proposed changes 😢 . You can clone my test project and compare it yourself.

Mac specs
Processor: 2.6 GHz 6-Core Intel Core i7
Memory: 32 GB 2667 MHz DDR4

Typescript was taking ~2.3 seconds to infer and retrieve the keys (thousands of keys), and that number increased to
~26 seconds

@JCQuintas
Copy link
Contributor Author

@pedrodurek that is odd, I went to try it on my machine and results differed a lot from yours, and are actually on par with a previous test I did where compilation time increased about 10%. Maybe I'm doing something wrong?

tsc build tsc (osx) build (osx)
previous 1.5 ~ 1.6s 2.8 ~ 2.9s 2.3 ~ 2.4s 4.2 ~ 4.6s
new 1.7 ~ 1.8s ~3.1s 2.4 ~ 2.5s 4.5 ~ 5s

my steps were

  • yarn > yarn tsc > yarn build
  • use my local branch package.json.dependencies.react-i18next: link:../react-i18next > yarn > yarn tsc > yarn build

I also tried adding some keys with actual contextSeparators like key01.subKey01.subSubKey_02 to see if that was the issue, but got about the same results.

Can you check this out? My specs are as follows

OS: Ubuntu 20.04 focal(on the Windows Subsystem for Linux)
Shell: zsh 5.8
CPU: AMD Ryzen 7 3800X 8-Core @ 16x 3.893GHz
RAM: 8197MiB / 25622MiB
OS: macOS 11.4 20F71 x86_64 
Shell: zsh 5.8 
CPU: Intel i7-9750H (12) @ 2.60GHz 
Memory: 11052MiB / 16384MiB 

@pedrodurek
Copy link
Member

Hey @JCQuintas, sorry I should have clarified a lit bit more.

The actual build time didn't suffer much (around 10%, as you explained), the problem lies in the TS Server from VScode (or similar) when inferring (autocompleting/ts check) keys/return type for you. In order to test it, you need to enable the Ts Server log, you can find more info about that here.

Before:
Screenshot 2021-07-20 at 4 38 57 PM

After:
Screenshot 2021-07-20 at 4 32 02 PM

Screen.Recording.2021-07-20.at.4.49.42.PM.mov

@JCQuintas
Copy link
Contributor Author

@pedrodurek AH! I see, yeah that is an issue. I don't have any immediate fix for that, and I suppose it would require some amount of time to nail out the issues and try to simplify the inference, if at all possible.

Should I close this PR then?

@pedrodurek
Copy link
Member

I'd keep it open until we find a way to improve that. What do you think @adrai ?

Thanks @JCQuintas btw! 🙌

@reichhartd reichhartd mentioned this pull request Nov 29, 2021
5 tasks
@adrai
Copy link
Member

adrai commented Aug 6, 2023

@JCQuintas should be addressed with i18next v23.4.0 via i18next/i18next#2006

@adrai adrai closed this Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translation function fully type-safe not compatible with context
4 participants