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

implement #1311: add new syntax for interpolation of complex types #1316

Merged
merged 13 commits into from
May 25, 2021

Conversation

cellog
Copy link
Contributor

@cellog cellog commented May 9, 2021

implements #1311

Note: I assume the repo for the docs is in a separate place? I augmented the example app, but would need to update the docs as well, should the PR look good. I plan to do that as a pre-step to release if that's cool.

This is a SUBSTANTIAL addition to the API, but does not break any existing functionality. It's also worth noting that it doesn't actually change anything runtime, because all of these additions only have an effect if used inside Trans.

It might be worth adding a check and error if any of these suckers are used OUTSIDE of a Trans, but this PR doesn't do that yet. I wanted to be sure we have a clear agreement on the structure of this new feature.

@jamuhl for your consideration.

Sample old way:

      <Plural
        i18nKey="testKey"
        count={itemsCount3}
        $0={<Trans>There is <strong>no</strong> item.</Trans>}
        one={<Trans>There is <strong>#</strong> item.</Trans>}
        other={<Trans>There are <strong>#</strong> items.</Trans>}
      />
      <Trans>Caught on { catchDate, date, full }</Trans>

Sample new way:

      <Trans i18nKey="testKey">{plural`${itemsCount1},
        =0 {There is ${(<strong>no</strong>)} item.}
        one {There is ${(<strong>#</strong>)} item.}
        other {There are ${(<strong>#</strong>)} items.}
      `}</Trans>
      <Trans>Caught on {date`${catchDate}, full`}</Trans>

The reason I chose to implement it this way was for typescript: if we do this, we can add an additional check that the interpolated variable is a Date instance for date/time, and for numbers/plural/selectOrdinal, we can check for number type, and for select we can check for a string type.

Sample of what I have in mind for the typing:

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgIilQ3wG4AoczAVwDsNgJa4ATFGJAHgBUA+ACgDOMKMFoBzQQC443JOAA27JAGURYyQEEoUFAE8ANHABuKUSgBGCpDIAiyowDpnZyTJS09AbQC6ASjgAb3I4ULgiGGooZgByAAtgGPIAXyo6BiY4GFAuPiF1CWlZeTAlDjVRQu1dQxMzYEtrOwc4Z0dXIo9vfyCQsIio2ISk1Jp6bMzaahALJCgeAWFKt2LFZQqNQWr9I1NzKxs4KZm5pxcoFa7fAOCw8KRI6Lh4xJS08cZmUqiUBQX85ZFORrcoFLQ6HZ1fZNI7TWZQM7tC6dTzXXp3AZPF4jd4ZZiCJDWDD-JabGTA0rrMFbCG1PYNA4yUkSREddyonq3foPQbPYZvchoJjCODAQT2DhwAC8RyQAHc4BKkBQhbQRWKAHLTaVwABMKuF8DFGwkOvwCTIlDYHAABgASQJipXJIyCOLQGA2ijWpD2x2CLUgZJe8g+v3GsHBijkY7w8Pi5RRmNwubxwNJ2Oph0R5ZJ8jfXQKePOowQTCYAkwKS60swOJzIJwQS4JBZXRqsqfOAOzhClhIXgAVk4AHo+wPknAkwXfmnpi64GWKw9q7X61BG82QK2RB5BJ3Mj3x0PR8fJ9OFD8i9nBCbxAul5XV4u6w3Ak2W229wfmEeIP2TzHf8JynENyAJIlPRvEs4ASRtMA8NAxCQQRBFAigIKQDA5yDIw4PfBD6GQ1D0PAwksKg-07wXfC4EIpDaBQtCkyAA

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

@cellog
Copy link
Contributor Author

cellog commented May 9, 2021

note: I can't reproduce that snapshot test failure locally.

I can fix the codeclimate stuff once I have a ✅ on the design choices.

all better now

@coveralls
Copy link

coveralls commented May 10, 2021

Coverage Status

Coverage increased (+0.3%) to 96.564% when pulling b1b22e3 on cellog:interpolation into 272f321 on i18next:master.

@jamuhl
Copy link
Member

jamuhl commented May 10, 2021

@cellog looks good to me -> will the old variation still work - or will this need to be published as major version changing the API

@cellog
Copy link
Contributor Author

cellog commented May 10, 2021

@cellog looks good to me -> will the old variation still work - or will this need to be published as major version changing the API

I didn't modify any of the existing tests, and they all pass, so based on that information I do believe this is fully backwards-compatible.

Things I want to add before I make it ready for review:

  • typescript types. Now that the PR adding types is merged, I can go ahead and do this, yay!
  • more robust checking. It should error if any of these new things are used outside of Trans at compile-time, for instance.
  • tests for negative cases (bad syntax). All the tests right now are passing, I'd like to add a few error tests as well, both for the new and the old syntax.

@cellog cellog marked this pull request as ready for review May 11, 2021 03:16
@cellog
Copy link
Contributor Author

cellog commented May 11, 2021

ok @jamuhl here's what I propose: if you like what you see, I can also do a PR against react-i18next-gitbook for the docs. Thanks for bearing with me in this! Took a little while to get the codeclimate thing to be happy.

@cellog
Copy link
Contributor Author

cellog commented May 11, 2021

also, I would LOVE to do a follow-up PR that refactors icu.macro.js to split it up into separate files, and perhaps clean up some of the use of global variables to make it a bit more maintainable, if you're game

@jamuhl
Copy link
Member

jamuhl commented May 11, 2021

@cellog sure...any help is very helpful...even more with the macro, icu support, ... (which I don't use myself - so don't have the deepest understanding)

@cellog
Copy link
Contributor Author

cellog commented May 15, 2021

@jamuhl just so I'm clear: are you waiting for any follow-up commits from me prior to reviewing this PR? Thanks

@jamuhl
Copy link
Member

jamuhl commented May 17, 2021

@cellog no, sorry was rather busy with other stuff...I wasn't able to follow this close enough -> so I'm more or less an "ok" from your side that this is ready for review and merge/publish

@cellog
Copy link
Contributor Author

cellog commented May 17, 2021

ok, I will do a round of extensive testing and let you know when it's ready. Thanks Jan!

@cellog
Copy link
Contributor Author

cellog commented May 17, 2021

@jamuhl this is good to go. I'll get started on the documentation PR this week

@cellog
Copy link
Contributor Author

cellog commented May 21, 2021

@jamuhl this and the documentation are good to go, release whenever ready!

jamuhl added a commit to i18next/react-i18next-gitbook that referenced this pull request May 25, 2021
@jamuhl jamuhl merged commit 5fefd9e into i18next:master May 25, 2021
@jamuhl
Copy link
Member

jamuhl commented May 25, 2021

published in v11.9.0

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