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(core/i18n/t): Precise typing for message parameters #1840

Closed
wants to merge 24 commits into from

Conversation

dilame
Copy link

@dilame dilame commented Jan 22, 2024

Hello @lingui/core enthusiasts!

I'm thrilled to introduce a groundbreaking enhancement that's set to transform your experience with the i18n.t() function. This PR is all about injecting steroids into type safety, ensuring that every parameter in your translation strings is strictly typed and meticulously validated. Say goodbye to loose strings and hello to precision!

Key Changes:

  1. Strict Parameter Validation:

    • You can now call i18n.t() with a single string argument only if the string does not contain parameters. For example:
      i18n.t('Hello world'); // ✅ All good!
    • If a string contains parameters, passing it directly will trigger a compilation error:
      i18n.t('Hello {name}'); // ❌ Compilation error
    • Parameter objects must strictly match the extracted parameters, no more, no less:
      i18n.t('Hello {name}', {username: 'Dmitry'}); // ❌ Compilation error
      i18n.t('Hello {name}', {name: 'Dmitry', lastName: 'Slivaev'}); // ❌ Compilation error - Object literal may only specify known properties, and lastName does not exist in type { name: string; }
      i18n.t('Hello {name}', {name: 'Dmitry'}); // ✅ Perfect match!
  2. Advanced Parameter Extraction:

    • Utilizing the I18nT type, parameters are extracted directly from the string, paving the way for a sophisticated, error-proof translation workflow. For instance:
      type ExtractedParams = I18nT<`{numBooks, plural,
        one {{numArticles, plural,
          one {1 book and 1 article}
          other {1 book and {numArticles} articles}
        }}
        other {{numArticles, plural,
          one {{numBooks} books and 1 article}
          other {{numBooks} books and {numArticles} articles}
        }}
      }`>;

Results in

{
      numBooks: string;
      numArticles: string;
}

The Magic Behind:

  • Precision in Internationalization: This enhancement brings an unprecedented level of precision to your internationalization efforts. Misplaced parameters or mismatched types? Not on our watch!
  • Seamless Developer Experience: With intelligent code completion and instant error detection, your i18n workflow is about to get smoother and more efficient than ever.
  • Future-Proof Foundation: While the current focus is on string parameters, this groundwork sets the stage for potential future enhancements, possibly incorporating specific types based on formatters.

Your Feedback is Our Catalyst:

This enhancement is a testament to our commitment to excellence and innovation. But it's your insights, feedback, and active participation that will fine-tune this feature to perfection. Dive into the code, test it out, and let's make i18n.t() not just a function, but a powerhouse of precision and reliability.

I'm eagerly awaiting your valuable feedback and suggestions!

Copy link

vercel bot commented Jan 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
js-lingui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 26, 2024 7:53pm

Copy link

github-actions bot commented Jan 22, 2024

size-limit report 📦

Path Size
./packages/core/dist/index.mjs 2.81 KB (0%)
./packages/detect-locale/dist/index.mjs 723 B (0%)
./packages/react/dist/index.mjs 1.67 KB (0%)
./packages/remote-loader/dist/index.mjs 7.26 KB (0%)

@timofei-iatsenko
Copy link
Collaborator

So impressed about what typescript type system is capable of.

However, the primary and recommended usage of lingui is via Macro (this is the only way to receive all goodies from lingui) which is literally eliminating the issue:

const username = 'Dmitry';
t`Hello ${username}` // <-- extracted as `Hello {username}`

I really like what you did and appreciate the effort, but, i'm a bit concerned about maintenance burden and complexity of the solution for a little value (considering primary macro usecase).

@dilame
Copy link
Author

dilame commented Jan 22, 2024

First and foremost, I want to extend my sincere gratitude for your time and the insightful feedback you provided.

I appreciate your point about the primary usage of Lingui being via the Macro system. However, I'd like to share some context regarding my use case, which I believe might shed light on why the enhancements proposed in the PR could be beneficial to a subset of Lingui users, particularly those working on server-side applications.

As a developer of a server-side application, my choice to use Lingui was driven because it's written in TypeScript and the simplicity offered by the i18n.t function. While I recognize the merits of i18next, it is written in JavaScript and its TypeScript typings do not meet the same quality standards, making Lingui a single suitable option.

I understand that the Macro system is the recommended approach. However, as a user, my priority is to adopt solutions that align with the project's needs and technical constraints. For my use case, employing a pure TypeScript compiler (TSC) without introducing additional bundlers like Vite or SWC is paramount. The necessity to integrate these tools solely for Macro processing introduces a layer of complexity and potential technical overhead that I am keen to avoid.

Furthermore, a glance at the npmjs statistics indicates a noteworthy trend: @lingui/core has 190,080 weekly downloads, while @lingui/macro has 116,385. This suggests that a significant number of developers might be favoring the core functionality, possibly due to similar constraints or preferences in their development workflows.

The two full working days I invested in crafting this solution were not due to a lack of awareness of the Macro system. Instead, it was a deliberate decision to enhance the typing mechanism within the core package to cater to use cases like mine, where the introduction of additional bundlers (limited by lingui with just Babel and SWC) is not an optimal path.

While I understand the concerns about the maintenance burden and complexity of the solution, I believe this enhancement has the potential to serve a meaningful segment of the Lingui community. It offers a way to leverage the library's powerful features while accommodating different project setups and developer preferences.

Once again, thank you for considering this contribution. I am looking forward to your thoughts and guidance on the next steps.

@dilame
Copy link
Author

dilame commented Jan 22, 2024

Also, I've been exploring various solutions, including typesafe-i18n, which offers an intriguing approach to i18n, aligning closely with my requirements. It emphasizes type safety and simplicity, leveraging a pure CLI without the need for bundlers or transpilers. However, despite its promising features, typesafe-i18n currently faces challenges such as lack of ESM support and some critical bugs and shortcomings. I attempted to contribute to resolving these issues but learned about the unfortunate passing of the library's sole developer, which deeply saddened me and the community (ivanhofer/typesafe-i18n#757 (comment)).

The popularity of typesafe-i18n underscores a clear demand in the market for an i18n solution that combines type safety with simplicity, steering clear of transpilers and bundlers. This is a niche that Lingui has the potential to fill effectively, and my proposed enhancements are a step in that direction. By offering similar type safety guarantees and ease of use, Lingui could become the go-to choice for the users who previously relied on or considered typesafe-i18n.

My contribution aims to bridge this gap, ensuring that Lingui not only maintains its robust features and developer-friendly approach but also caters to the evolving needs of the wider developer community.

@dilame
Copy link
Author

dilame commented Jan 22, 2024

BTW, while the higher download count of @lingui/core compared to @lingui/macro may indicate a preference within the existing Lingui community, it's also essential to consider the perspective of those evaluating i18n solutions. Many developers, including myself, have faced the challenging decision of choosing the right tool for our projects. In this decision-making process, the lack of built-in, hassle-free typing support can be a significant factor.

After the unfortunate realization that I couldn't use typesafe-i18n, I spent weeks comparing Lingui and FormatJS. Both offered comparable functionality, but Lingui resonated with me due to its "close to the ground" and straightforward approach (in @lingui/core package). However, the deciding factor for many, including myself, could have been the type of robust, built-in typing support I've proposed in this PR. It eliminates the need for additional transpilers, striking a perfect balance between functionality and simplicity.

By incorporating these typing enhancements, Lingui has the potential to position itself as a unique and compelling choice in the i18n market. It's not just about retaining the existing user base but also about attracting those who might have overlooked Lingui due to the perceived complexity of achieving type safety. This enhancement could be a game-changer, significantly broadening Lingui's appeal and encouraging broader adoption.

I firmly believe that these enhancements will not only meet the current users' needs but also resonate with a wider audience looking for an i18n library that marries functionality with ease of use and type safety.

@timofei-iatsenko
Copy link
Collaborator

Ahh, now i see where this idea came from. Well, as i mentioned i'm not against your changes and always striving to the most type-safety in my projects, just raised a concern about maintainability. It isn't a secret that this particular kind of typings are extremely hard to read and understand and not many developers can and would do that. So in the end of the day we may stuck with something broken and no one understand what's going on and how to fix it.

@timofei-iatsenko
Copy link
Collaborator

Regarding the PR, we're using tsd for type testing. I think you should add more tests covering happy and unhappy paths with typings. You could check packages/macro/__typetests__/index.test-d.tsx for reference.

@timofei-iatsenko
Copy link
Collaborator

And also would be nice to update website and highlight this feature.

@dilame
Copy link
Author

dilame commented Jan 22, 2024

Thank you @thekip for your supportive feedback and for the opportunity to contribute to this exciting enhancement. I'm glad to hear that we're moving in the right direction.

Regarding your suggestions:

  1. Type Testing with tsd: I completely agree on the importance of comprehensive type testing to ensure the robustness of the new typing mechanism. I will diligently work on adding a series of tests covering both happy and unhappy paths, drawing inspiration from the existing tests in packages/macro/__typetests__/index.test-d.tsx and packages/core/__typetests__/index.test-d.tsx. Ensuring that the new enhancements are well-tested is a priority for me, and I'm eager to get started on this.

  2. Updating the Website: I understand the significance of properly documenting and highlighting this new feature on the website. However, I must admit that website updates are not my strongest suit. While I won't be able to take on the website update myself, I fully recognize its importance. Would it be possible for another team member who is more experienced in this area to handle the website updates?

@timofei-iatsenko
Copy link
Collaborator

Updating the Website: I understand the significance of properly documenting and highlighting this new feature on the website. However, I must admit that website updates are not my strongest suit. While I won't be able to take on the website update myself, I fully recognize its importance. Would it be possible for another team member who is more experienced in this area to handle the website updates?

I think @andrii-bodnar and myself could help with that. I think in general we could start by adding a separate page "Using in backend" and explain how to use without macro, what are strong sides and so on. Furthermore, I think we could add "fully type safe" as a highlight on the index page between other highlight there.

Comment on lines 2 to 3
// eslint-disable-next-line import/no-extraneous-dependencies
import { Replace, Simplify, Trim, UnionToIntersection } from "type-fest"
Copy link
Collaborator

Choose a reason for hiding this comment

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

if you use webstorm and see warning from eslint from this rule that's probable because webstorm's eslint service should be restarted. It doesn't pick up changes in package.json automatically and continues to show error even if the package is already added. However this is not the case when you run eslint command directly.

@dilame
Copy link
Author

dilame commented Jan 23, 2024

I'm excited to share some additional enhancements I've made

Strict Typing for Formatters

In pursuit of providing a more robust and error-resistant internationalization experience, I have implemented strict typing for formatters. This means that the library is now even more intelligent in understanding and enforcing the types of variables passed to formatters.

Previously acceptable but erroneous usages like this are now rightly identified as errors during compilation.

i18n._("You have {n, number} unread messages", {n: 'hello'})

Correct usages such this are seamlessly validated, ensuring that the data types align with the expectations of the number formatter.

i18n._("You have {n, number} unread messages", {n: 10})

This enhancement is not limited to number formatters but extends to all formatters.

Types for formatter inputs are automatically inferred from the actual formatter types. This means there is a single source of truth for the expected types, minimizing maintenance overhead and eliminating the need for manual synchronization. The system is smart enough to understand and enforce the types directly based on the formatters themselves.

Comprehensive Testing and Bug Fixes

Following your valuable suggestion, I've expanded the test suite, adding more cases to ensure the robustness of the new typing mechanism. This testing process helped unearth and rectify several bugs, further solidifying the reliability of the implementation.

I'm open to suggestions if there are any specific cases or scenarios you think might need additional attention.

Thank you once again for your support and guidance in this endeavor.

@dilame
Copy link
Author

dilame commented Jan 23, 2024

Regarding the tests, i consciously chose not to overload the test suite with cases involving deliberately incorrect message syntax. This is because, in runtime, such syntax should rightfully throw errors, and during compile-time, our capacity to convey formalized error messages is somewhat limited. Consequently, I believe that encountering unpredictable parsing results at compile-time with inherently incorrect syntax is an acceptable trade-off.

@dilame
Copy link
Author

dilame commented Jan 23, 2024

It isn't a secret that this particular kind of typings are extremely hard to read and understand and not many developers can and would do that.

I completely agree that reliability in software often necessitates a more complex underpinning than its less dependable counterparts. Yet, I've made a concerted effort to ensure that the complexity of this solution is manageable and not as daunting as it might initially appear. I've endeavored to make the code as comprehensible as possible within the syntax constraints of TypeScript. This includes giving types, generics and variables meaningful names and decomposing the parser into the smallest feasible modules, each with a clear and descriptive name. The intention here is to make the logic as transparent and approachable as possible, even for developers who might not be deeply versed in TypeScript's advanced type system.

So in the end of the day we may stuck with something broken and no one understand what's going on and how to fix it.

In the event that any issues arise, or if certain aspects of the implementation are unclear, I want to extend an open invitation for direct communication. Feel free to reach out to me on Telegram, and I will do my utmost to provide clarity or assistance.

Copy link

codecov bot commented Jan 26, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (85f0944) 76.39% compared to head (e42c01e) 76.36%.

❗ Current head e42c01e differs from pull request most recent head 3fc4562. Consider uploading reports for the commit 3fc4562 to get more accurate results

Files Patch % Lines
packages/core/src/i18n.ts 66.66% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1840      +/-   ##
==========================================
- Coverage   76.39%   76.36%   -0.04%     
==========================================
  Files          81       81              
  Lines        2072     2073       +1     
  Branches      529      530       +1     
==========================================
  Hits         1583     1583              
  Misses        377      377              
- Partials      112      113       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

For beautiful "Inlay Hints" for Parameter names in IDE. Previously IDE showed "id" even when you called it with descriptor. Now it doesn't show the first parameter name at all, which is more logical
@dilame
Copy link
Author

dilame commented Jan 26, 2024

I renamed parameter id to '_' in overloads for beautiful "Inlay Hints" functionality for "Parameter names" in IDE. Previously IDE showed id even when you called it with descriptor. Now it doesn't show the first parameter name at all, which is more logical

@andrii-bodnar
Copy link
Contributor

This looks impressive but I agree with @thekip's concern about maintenance burden.

Personally, I believe that it's a good candidate for a new feature of the Lingui ESLint plugin instead of bringing such complexity into the library core. For example, as it is done in the eslint-plugin-formatjs - https://formatjs.io/docs/tooling/linter/#enforce-placeholders

Or we can use the declaration merging and module augmentation features of TypeScript and keep it as a separate package.

In addition, it contains breaking changes that can't be released without bumping a major release, which we'd like to avoid for now.

@dilame
Copy link
Author

dilame commented Feb 9, 2024

@andrii-bodnar addressing your concerns, I'd like to present a structured argument against that perspective:

  1. Correcting Data Types, Not Breaking Changes: This PR enforces the use of correct data types. If users previously passed incorrect types, those were errors that this PR aims to correct. Correcting errors should not be considered as introducing breaking changes.
  2. Full Coverage of Current Functionality: The modifications I've proposed cover 100% of lingui's existing functionality, and, given the comprehensive nature of lingui's current features, it's unlikely that the syntax parser will need revisions in the near future. And, in the end of the day – we are all developers here, and it's just a code:)
  3. Seamless Integration of New Formatters: My changes allow for the easy integration of new formatters without additional effort. Developers can write formatters, and the engine will automatically handle data types.
  4. Value Addition Through Complexity: The proposed feature adds significant value by leveraging TypeScript for better type safety. This solution addresses a critical aspect of software development that TypeScript handles exceptionally well.
  5. Native Solution Versus ESLint Plugin: While an ESLint plugin could be developed, it wouldn't match the efficiency or effectiveness of this native solution, which is also ready implemented in this PR. I am committed to assisting with any challenges that arise from this implementation.

This pull request represents a significant step forward in making lingui a unique and pioneering product in the TypeScript i18n space, and possibly beyond. It's about enhancing type safety in our applications, a core principle that no one manages better than TypeScript. I kindly request a reconsideration of the advantages offered by this approach. You will get a unique product on the market, i will get i18n type safety for my server-side project:)

@dilame
Copy link
Author

dilame commented Feb 9, 2024

Oh, and a regarding the ESLint plugin, i don't think that an ESLint-based approach can achieve 100% functionality of this TypeScript-based solution. This is because my solution inherently considers the data types of formatters, i'm not sure it can be fully replicated in ESLint without duplicating data types in both TS compile-time and ESLint runtime, which violates the principle of a single source of truth. For instance, specific formatters like plural require a number, while date expects a string, and it is defined in TypeScript types. My parser directly utilizes these definitions, a feat not achievable with an ESLint plugin.

@andrii-bodnar
Copy link
Contributor

Correcting Data Types, Not Breaking Changes: This PR enforces the use of correct data types. If users previously passed incorrect types, those were errors that this PR aims to correct. Correcting errors should not be considered as introducing breaking changes.

It's a breaking change since the exported types have changed (MessageDescriptor, MessageOptions, PluralOptions, Formats). Developers might be using these types.

We appreciate the effort, but let's consider a compromise solution as a separate package or an Eslint-based option. There is probably no need for such precise type validation. We are trying to keep the balance between simplicity and usability, and this PR definitely adds more complexity.

Copy link
Collaborator

@vonovak vonovak left a comment

Choose a reason for hiding this comment

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

Thank you for the effort! I'm a fan of type safety but agree that this adds a lot of complexity.

It'll make it harder for TS-non-experts to contribute.

That being said, I'd love to have some compromise, maybe declaration merging could help?

https://www.typescriptlang.org/docs/handbook/declaration-merging.html

Also, I might be wrong, but it seems you're writing your responses with the help of some AI. Can you instruct it to write more concise messages? The responses feel a bit too long. Thank you!

| ExtractVars<Next> // Recursively process the rest of the string after the current variable.
: // If not a formatter, create a type with a property where the key is the whole trimmed BraceBody and the value is a string
{ [P in Trim<BraceBody>]: string } | ExtractVars<Next> // and recursively process the rest of the string.
: {} // If the string does not contain a valid structure, return an empty object type (possibly we can return an error here, because this branch indicates parsing error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

{} does not represent an "empty object" in TS, so ExtractVars might be behaving differently from what you expect

see https://www.totaltypescript.com/the-empty-object-type-in-typescript

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thank you for pointing that out! I know about this specific issue, and it's actually the only place I also wasn't sure about :) In this context, it makes no difference because ExtractVars only applies if the input string contains interpolation. But in the general case, I should use never here, and it should be fixed, of course. Thank you again!

@andrii-bodnar andrii-bodnar marked this pull request as draft February 16, 2024 08:07
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.

4 participants