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

Add proper TS typing to @wordpress/i18n #68906

Open
danieliser opened this issue Jan 27, 2025 · 6 comments
Open

Add proper TS typing to @wordpress/i18n #68906

danieliser opened this issue Jan 27, 2025 · 6 comments
Labels
[Package] i18n /packages/i18n [Type] Code Quality Issues or PRs that relate to code quality

Comments

@danieliser
Copy link
Contributor

danieliser commented Jan 27, 2025

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 example sprintf 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

/**
 * Base namespace for WordPress i18n types
 */
export namespace WordPress {
	/**
	 * Base interface for text domains.
	 */
	export interface TextDomains {
		'default': never;
	}

	// Add this type alias for better type inference
	export type TextDomain = keyof TextDomains;
}

// Export the type separately to make it easier to extend
export type TextDomain = WordPress.TextDomain;

export const sprintf = (
	format: string,
	...args: Array< string | number >
) => {
	return i18n.sprintf( format, ...args );
};

export const __ = ( text: string, domain: TextDomain ) => {
	return i18n.__( text, domain );
};

export const _x = ( text: string, context: string, domain: TextDomain ) => {
	return i18n._x( text, context, domain );
};

export const _n = (
	single: string,
	plural: string,
	number: number,
	domain: TextDomain
) => {
	return i18n._n( single, plural, number, domain );
};

export const _nx = (
	text: string,
	context: string,
	number: number,
	domain: TextDomain
) => {
	return i18n._nx( text, context, number, domain );
};

export const isRTL = () => i18n.isRTL();

And in your consumer

declare module '@wordpress/i18n' {
	namespace WordPress {
		interface TextDomains {
			'popup-maker': never;
		}
	}
}

Then when you call __( '', 'something-else' ); or sprintf( '%s %d', { text: 'string' }, { number: 0 } ) it throws errors on the second arg.

@danieliser danieliser added the [Type] Enhancement A suggestion for improvement. label Jan 27, 2025
@danieliser danieliser changed the title Add proper TS typing to wordpress/i18n Add proper TS typing to @wordpress/i18n Jan 27, 2025
@swissspidy swissspidy added the [Package] i18n /packages/i18n label Jan 27, 2025
@swissspidy
Copy link
Member

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.

@t-hamano t-hamano added [Type] Code Quality Issues or PRs that relate to code quality and removed [Type] Enhancement A suggestion for improvement. labels Jan 28, 2025
@swissspidy
Copy link
Member

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.

@danieliser
Copy link
Contributor Author

@swissspidy Appreciate the feedback.

Your sprintf suggestion is pretty clever there, but the ones suggested in the linked issue are better still and the suggested replacement package that is fully typed even better.

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.


So saying there are no types is just wrong.

Generated types are not really typed. They are inferences at best. Worse yet some of the JSDocs are intentionally incomplete.

I would argue an inferred type of any is worse than no type at all (unknown). Its ambiguous and leads to issues that Typescript is literally built to solve before they occur.

A strongly typed implementation could make this more robust.
~swissspidy

I have used __ & sprintf for years now in our TS admin stuff, but in all that time I never realized that it failed type checks entirely with those any on everything.

I noticed issues when I recently kept getting strings with [Object, object] in them. That shouldn't happen with TS, it should be flagging that I'm passing a known object in to something that shouldn't accept one.

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:

Typing the text domain seems a bit overkill to me, I think handling that with ESLint is fine.

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:

  • Make this specifically about typing the other gettext functions with textdomain enforcement.
  • Reiterate need for sprintf typing and defer to the existing issue, which really needs to move forward using the new package.

@swissspidy
Copy link
Member

I have used __ & sprintf for years now in our TS admin stuff, but in all that time I never realized that it failed type checks entirely with those any on everything.

The only type I see with any is sprintf, which others am I missing?

@im3dabasia
Copy link
Contributor

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.

  • There's an ongoing discussion about replacing sprintfjs with @tannin/sprintf, which depends on when this PR gets merged in Tannin, which in turn will help resolve issues related to stricter type checking.

  • There's also a discussion about rewriting the sprintf function in TypeScript to have more control over the arguments passed. #52985

  • Additionally, there's mention of rewriting the entire i18n package in TypeScript. Quoting the place I read it from:

    "If you want to rewrite the package in TS, go for it :)"

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!

@swissspidy
Copy link
Member

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 TextDomain part needs some discussion I'd say.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] i18n /packages/i18n [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

No branches or pull requests

4 participants