feat: new autocompletion overload#2319
Conversation
|
Despite my closing remarks, I am still optimistic that we can find a solution to this problem. I have another idea for how we might be able to tackle this, although I'm in the process of thinking through some of the details. If my idea turns out to be viable, I believe it would be a more satisfying (and more general) solution to this problem. I'll open that PR in the coming days, if it turns out to be as simple to implement as I'm hoping. |
|
Hi! Thanks a lot for opening this PR — really appreciate your work on this. Just a heads-up: the CI won’t run until the conflicts with the base branch are resolved. At the moment, I don’t have the capacity to do a full review, especially since this looks like a complete rewrite of the typings and includes some missing features. I totally see the value in improving the DX, but we also need to make sure we don’t lose type accuracy or coverage in the process. Your POC is looking promising though! It might be a good idea to reach out to the maintainers about possibly merging this into a separate branch. That way, it could be published under an experimental tag for testing and feedback without affecting stable releases. Thanks again for the effort — looking forward to seeing how this evolves! |
| : never | ||
| : Keys; | ||
|
|
||
| export type ParseKeys< |
There was a problem hiding this comment.
Beware about removing these exports some plugins / users might be using it.
ParseKeys is extremely useful also in user land
There was a problem hiding this comment.
Ah, this was my IDE I think.
This is a quirk of *.d.ts files -- they behave like ambient namespaces (other way around actually, ambient namespaces behave like declaration files) in that all types that they contain are implicitly exported.
I'm happy to revert these lines, but just know that all types in t.d.ts are available to be used in userland.
There was a problem hiding this comment.
I'm happy to revert these lines, but just know that all types in t.d.ts are available to be used in userland.
Yes I know... they were always exported to userland.
At some point they were moved into separate files to support different version of typescript.
Check the following diff https://github.com/i18next/i18next/pull/2007/files#diff-7aa4473ede4abd9ec099e87fec67fd57afafaf39e05d493ab4533acc38547eb8
and the following commit from adrai: fba8527
| * See also: | ||
| * - {@link LazyNonStrict `TFunction.LazyNonStrict`} | ||
| */ | ||
| interface LazyStrict<Ns extends Namespace = DefaultNamespace, KPrefix = undefined> { |
There was a problem hiding this comment.
Since you asked about TFunction KeyPrefix you should consider that is used also on plugin side. For example on react-i18next.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Thank you for the review @marcalexiei -- just to clarify, in its current state, this PR is an additive change only -- none of the existing types were changed. This PR simple adds a new overload that only applies when the user does not pass an options object, since I don't think it's possible to get the autocomplete behavior in cases where the set of available keys depends on which options are passed. Either way, I'd say don't worry about doing a full review for now -- I'm working on validating another approach that I think would give the maintainers a more interesting set of tradeoffs to choose between. |
|
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. |
|
Closed in favor of #2322 |
Would close #2317
This is a draft PR that I'm opening to get feedback on a potential solution to the problem outlined in #2317.
Problem
Currently when users use i18next with TypeScript, the compiler needs to do a significant amount of work upfront in order to type check even a single call to the
tfunction.To measure this cost, I used a tool called @arktype/attest that tracks the number of allocations the TypeScript compiler needed to perform in order to type check a given expression.
Importantly, these instantiations continue to grow over time as users inevitably add more translations; in one example (included in this PR), a translation set of ~3,000 keys created over 800,000 instantiations.
Not only does a larger set of instantiations cause IDE performance to drag, but since these instantiations are stored on the heap, this particular performance characteristic tends to interact poorly when used in conjunction with other tools such as
@typescript-eslint, and can even cause OOM crashes in CI/CD pipelines (this issue is what originally motivated me to work on this problem).Bugs like these are particularly insidious since their reproduction depends heavily on environmental factors. This makes them difficult to debug, and nearly impossible to reproduce.
Proposed solution
This PR attempts to solve this problem by making path computation lazy.
It does so by using a bit of TypeScript magic to mimic the native autocompletion API.
Usage looks like this:
Benefits
For example, the same 3,000 keys mentioned above created only 771 instantiations, which is a 1,000x reduction
As translation sets continue to grow, "full-path" autocompletion becomes less ergonomic.
To demonstrate this, here's an example of ~1,800 translations with the current API:
Arguably, translations become more discoverable with the proposed API:
Limitations
This approach is not without its limitations.
The reason for this is complicated, but tl;dr: TypeScript sometimes get confused when the 1st argument (the key) depends on the value of the 2nd argument (the options object).
I spent considerable time getting it to work with context, specifically, and while it did mostly work, in some cases autocompletion broke, and I needed to restart the TS language server to get it to work again.
When @adrai tagged me in #2318 and I realized that we might also need to support the
keyPrefixoption, I changed my strategy for this PR.Therefore, in its current state, this PR only covers the case where a user does not pass an options object.
As soon as a user passes options, autocompletion falls back to eager resolution.
Furthermore, since users can choose to "stop" at any point in the object, this feature is only available to users when they select
returnObjectstotrue-- which means with this PR, this feature is currently disabled by default.TODO
There is still more to do if the maintainers would like to merge this PR.
Closing thoughts
Like I said in the beginning, I've opened this PR as a draft to get feedback, so if you have any thoughts or questions, please let me know!
In the end, since this change does not solve the entire problem, I'm personally on the fence about whether this is the best path forward. On the one hand, it does seem to improve DX, and definitely leads to far fewer allocations, but I also wouldn't want to ship this prematurely, only to discover that I'd missed an edge case that would be impossible to support, or arguably worse: that this feature ends up being costly for me (or other i18next contributors) to maintain.
Checklist
npm run testChecklist (for documentation change)