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

Replace typesafe-i18n with svelte-i18n #3302

Merged
merged 15 commits into from Nov 17, 2023

Conversation

pavish
Copy link
Member

@pavish pavish commented Nov 7, 2023

Technical details

  • This PR replaces typesafe-i18n with svelte-i18n.
    • Typesafe-i18n provides better type safety and functionality, however, it uses a custom format which introduces friction when connecting with Transifex.
    • Transifex expects the translation files to be on of their supported formats listed here.
    • The closest we can use for the svelte side is JSON with ICU Plurals which is a subset of the ICU format which is a well established format for i18n.
    • Using typesafe-i18n's custom format would result in:
      • having to train translators (and external translation services) in the format used by it, whereas the ICU format does not require this training
      • no direct support in the Transifex editor UI
      • errors when parsing the file within Transifex with typesafe-i18n's full syntax plural rule
      • we cannot convert the typesafe-i18n's custom format to a recognizable format for Transifex without having to implement a custom parser.
      • moving to other translation providers in the future might introduce additional friction we're currently not aware of.
    • svelte-i18n has all the required features, but it lacks type-safety.
      • It uses format.js under the hood, a popular i18n library, used to other tools like react-i18n.
      • The format used is ICU, so there's little to no friction when working with Transifex.
      • Moving to other translation providers or other libraries in the future would not be an issue, since the ICU format is widely accepted.
  • I've tested this PR end-to-end.
    • Here's a PR opened by Transifex bot containing ja translations.

Checklist

  • My pull request has a descriptive title (not a vague title like Update index.md).
  • My pull request targets the develop branch of the repository
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no
    visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@pavish pavish force-pushed the poc_replace_typesafe_i18n_with_svelte_i18n branch from c435a35 to 7ee3b43 Compare November 9, 2023 17:34
@pavish pavish marked this pull request as ready for review November 15, 2023 15:01
@pavish pavish added the pr-status: review A PR awaiting review label Nov 15, 2023
Copy link
Contributor

@seancolsen seancolsen left a comment

Choose a reason for hiding this comment

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

Really nice work on this, @pavish. I'm very happy with the direction this is heading!

Out of curiosity, I noticed that you moved tinro from dev-dependencies to dependencies. Why? I guess I would actually expect it to be in dependencies from the start. Were you observing a problem with it being in dev-dependencies? If so, what were the symptoms you were observing?

@seancolsen seancolsen added this pull request to the merge queue Nov 17, 2023
Merged via the queue into develop with commit c060af6 Nov 17, 2023
20 checks passed
@seancolsen seancolsen deleted the poc_replace_typesafe_i18n_with_svelte_i18n branch November 17, 2023 16:16
@pavish
Copy link
Member Author

pavish commented Nov 22, 2023

I noticed that you moved tinro from dev-dependencies to dependencies. Why? I guess I would actually expect it to be in dependencies from the start. Were you observing a problem with it being in dev-dependencies? If so, what were the symptoms you were observing?

For frontend projects with a build step, it usually does not matter if we have the dependencies in either one of these places. There was no problem with tinro being in dev-dependencies, but I moved it to dependencies mainly for the sake of making it clear that it is bundled with the app.

The only place where it would matter is when we run npm audit --production which will only check dependencies. Again, for frontend projects, it doesn't make much of a difference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-status: review A PR awaiting review
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants