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

feat(types): allow key separator augmentation #1367

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

ImADrafter
Copy link
Contributor

@ImADrafter ImADrafter commented Sep 1, 2021

ATM, type checking the dictionaries will not work properly if you have changed the keySeparator for javascript:

i18n.init({
  ...
  keySeparator: '>>>',
)}:

t('key>>>nested') // Will translate at runtime, but typescript will complain, as he is always expecting the default key separator. 

The changes that im proposing allows to augment the keySeparator, in order to maintain aligned the typescript interface and the i18n instance.

Nevertheless, im not happy with all the code changes cause i had to rewrite NormalizeReturn, and it became too complex to really understand all the chaining conditions, so if you have any clue of how it should remain simple, please let me know.

Thank you in advance for reviewing this changes.

Checklist

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

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.416% when pulling 9109e04 on ImADrafter:feat-augmeted-key-separator into 04b2bca on i18next:master.

@pedrodurek
Copy link
Member

Seems great to me, I'll just do some performance tests tomorrow to ensure compilation time won't be jeopardized.

@stale
Copy link

stale bot commented Sep 10, 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 Sep 10, 2021
@pedrodurek pedrodurek removed the stale label Sep 16, 2021
@stale
Copy link

stale bot commented Sep 23, 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 Sep 23, 2021
@stale stale bot closed this Oct 1, 2021
@adrai adrai removed the stale label Oct 1, 2021
@adrai adrai reopened this Oct 1, 2021
@stale
Copy link

stale bot commented Oct 8, 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 Oct 8, 2021
Copy link
Member

@pedrodurek pedrodurek left a comment

Choose a reason for hiding this comment

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

Hey sorry for the delayed response, packing and unpacking when moving provinces is not easy 😅 . PR is good to go!

Just for the record:
This PR is slowing down the compilation time by ~23% when inferring the keys for the first time (big projects).

Before:
Screenshot 2021-10-12 at 5 08 26 PM

After:
Screenshot 2021-10-12 at 5 03 00 PM

@pedrodurek pedrodurek removed the stale label Oct 13, 2021
@ImADrafter
Copy link
Contributor Author

Worry not @pedrodurek , thank you for taking the time after all :)

Im a little bit disappointed to see that compilation time increases, I think it shouldn't... Do you have any idea on how can we improve performance or can we move foward?

BTW, how can I test myself performance? are you using a project that you can share?

@pedrodurek
Copy link
Member

pedrodurek commented Oct 16, 2021

Hey @ImADrafter don't worry, it didn't increase much. That's normal, the more validation we add, the more time it'll take since it iterates over every key doing the same check.

I'm testing using this repo https://github.com/pedrodurek/i18n-type-tests, to test performance you need to enable the Ts Server log, you can find more info here.

@stale
Copy link

stale bot commented Oct 23, 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 Oct 23, 2021
@pedrodurek pedrodurek removed the stale label Oct 25, 2021
@adrai
Copy link
Member

adrai commented Oct 28, 2021

so ready to merge?

@pedrodurek
Copy link
Member

yep!

@adrai adrai merged commit fadddff into i18next:master Oct 28, 2021
@adrai
Copy link
Member

adrai commented Oct 28, 2021

included in v11.13.0

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.

4 participants