-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add proper TS typing to @wordpress/i18n #68906
Comments
As a contributor and heavy user of this package in TS code bases I can‘t really relate with this, tbh. Usage is super smooth. But if you want to rewrite the package in TS, go for it :) Better types for sprintf are discussed in #52985 Typing the text domain seems a bit overkill to me, I think handling that with ESLint is fine. |
By the way you can see the existing generated types at https://www.npmjs.com/package/@wordpress/i18n?activeTab=code So saying there are no types is just wrong. |
@swissspidy Appreciate the feedback. Your I’m surprised this issue still exists, given the prior discussions. Hopefully, we can keep moving forward now. It’s unfortunate we haven’t seen more quality-of-life improvements here. This isn’t meant as criticism toward any single individual, just an observation on the current state of the package.
Generated types are not really I would argue an inferred type of
I have used I noticed issues when I recently kept getting strings with Once I retyped it we found over a dozen instances in one code base where this had been overlooked. Most it seems we were lucky as the objects in question had toString and collapsed into a string gracefully. Granted this is a simple string based function, the impacts it has might be minuscule, but given @WordPress packages in general are now being used by even non wp projects, purely inferred types aren't ideal. I'd prefer no generated types in this case, at least I can add the missing types knowing their correct. Lastly re:
Dealers choice, but if thats the opinion, why do we have similar rules in ESLint rules or WPCS for php then either? Many TS projects don't use eslint by default or at all, so adding simple type enforcement could make this more robust as you say. Point in case, ESLint isn't even installed in VS Code & many IDE by default, but TS is. So in any given project an ESLint rule might not flag until CLI checks are run, but the IDE would highlight the TS error immediately. I think its also easy to make an assumption these packages are always used within WordPress, but that isn't the case these days. Further its easy to assume every project uses the same tooling, but I've just shown that also isn't neccessarily true either. Just my 2 cents and efforts to contribute. take it for what it is. We all want a robust, well-typed solution, so I appreciate everyone’s efforts in that direction. Given the issues/prs you linked I'm inclined to update this rather than close it:
|
The only type I see with |
Hey @swissspidy, Thank you @swissspidy for following up on this issue and linking the multiple related discussions. I see some 2/3 discussions within this issue.
If you were to suggest an order in which to approach these related issues, it would be really helpful in deciding where to start. Looking forward to your guidance! |
Points 1 & 2 are basically the same. I hope this can get resolved in Tannin so that we can switch to that improved implementation. This is a bit outside of our control (unless a TS wizard comes along and helps finish that Tannin PR). That leaves as with the last point, rewriting the package in TypeScript. That could be easily worked in separately from the sprintf discussion. No blockers there. It should be relatively straightforward. Only the |
What problem does this address?
Currently the
@types/wordpress__i18n
package is marked as deprecated and says the types are included in the@wordpress/i18n
package directly.Further there are no types, and the JSDocs types that exist now accept
any
in places it definitely should not. For examplesprintf
accepts objects for the...args
.Lastly proper enforcement of text domains is reasonably feasible with TS.
What is your proposed solution?
I'll propose the types or do the PR if someone will quickly work it up the chain for approval.
If it can get done quickly, I'll offer to retype some of the other common packages as we have built entire TS plugins at this point and had to deal with lots of
any
issues.Proposed solution
Here is my completely extendable TS wordpress/i18n functionality, including way to enforce custom text domains via addons/plugins
We are already using this currently in a custom internal
@plugin/i18n
package. It works great in all cases described.Problems Solved:
✅ Proper type enforcement on
__
,sprintf
and other args.✅ Throws errors when objects or non string|numbers are passed.
✅ Extendable from consuming plugins/themes
And in your consumer
Then when you call
__( '', 'something-else' );
orsprintf( '%s %d', { text: 'string' }, { number: 0 } )
it throws errors on the second arg.The text was updated successfully, but these errors were encountered: