Conversation
|
Size Change: 0 B Total Size: 6.84 MB ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
left a comment
There was a problem hiding this comment.
Thank you for addressing this issue so quickly!
Perhaps we may need to tweak the rules a bit more to prevent false positives.
As far as I can tell, the following code does not produce any warnings.
Calling a function defined outside the save function:
function renderLabel() {
return __( 'Label text', 'text-domain' );
}
function save() {
return <div>{ renderLabel() }</div>;
}When a function with a name other than save is used as the save operation for a block:
// index.js
function saveWithInnerBlocks() {
return __( 'Content', 'text-domain' );
}
registerBlockType( 'my-plugin/block', {
save: saveWithInnerBlocks,
} );I'm not sure how precisely this can be done, but it might at least be possible to check whether "there is a translation function defined in the callback function of the save property of registerBlockType()".
I'm not familiar with creating ESLint rules, but from consulting with the AI, it looks technically possible 🤔
|
P.S. The reason why the rule needs to be strict is that it also affects consumer projects that use the |
|
I think it would be hard to make this a general rule that applies to every consumer. For this project, we could disallow importing i18n methods in |
|
I see, so if we add the rules to the |
This is also my first time creating an ESLint rule, so I'm not sure if this is correct: I've attempted this in 0e1f51a, by moving the new rule from the eslint-plugin config and into the Gutenberg-specific eslintrc file 👀 |
There was a problem hiding this comment.
Pull request overview
Adds a new Gutenberg ESLint rule to prevent i18n translation calls inside block save implementations, helping avoid block validation issues caused by locale-dependent saved markup.
Changes:
- Introduces
@wordpress/no-i18n-in-saverule to flag__,_x,_n,_nxusage insavefunctions andsave.(js|ts|jsx|tsx)files (with an exception fordeprecated.*files). - Adds unit tests covering multiple
savedeclaration styles. - Adds rule documentation and enables the rule in the repo ESLint config.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/eslint-plugin/rules/no-i18n-in-save.js | Implements the new ESLint rule logic (file-based + save-function-based detection). |
| packages/eslint-plugin/rules/tests/no-i18n-in-save.js | Adds RuleTester coverage for the rule across multiple save patterns. |
| packages/eslint-plugin/docs/rules/no-i18n-in-save.md | Documents rationale and examples for the new rule. |
| .eslintrc.js | Enables @wordpress/no-i18n-in-save as an error in the repo ESLint config. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
scruffian
left a comment
There was a problem hiding this comment.
I get this error when I try to add a translation function to a save.js file:
Translation functions should not be used in block save functions. Translated content is saved to the database and will not update if the language changes @wordpress/no-i18n-in-save
t-hamano
left a comment
There was a problem hiding this comment.
Could we scope this rule to just the packages/block-library/src/** directory instead of applying it globally? It’s only relevant when building blocks and isn’t needed elsewhere.
Good idea, done in 2c4749b. |
|
Flaky tests detected in 4399554. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22447926035
|
t-hamano
left a comment
There was a problem hiding this comment.
Thanks for the update! Can we update the README as well?
gutenberg/packages/eslint-plugin/README.md
Lines 70 to 93 in 0f77b6e
Good spot! I've updated the readme in 90caaec. |
|
I've also removed the recommendation for enabling this rule in the docs in a recent commit (7bbe256). I thought it made more sense not to recommend that this be enabled by default when it is not enabled globally here. |
t-hamano
left a comment
There was a problem hiding this comment.
LGTM! Not a blocker, but what do you think about overriding the root level rules rather than creating a new .eslintrc.js file?
module.exports = {
// ...
overrides: [
// ...
{
files: [ 'packages/block-library/src/**/*.[tj]s?(x)' ],
rules: {
'@wordpress/no-i18n-in-save': 'error',
},
},
// ...
],
};
Good idea, I prefer this to adding a new .eslintrc.js file. Done in 4399554. Thanks! |
|
Thanks for working on this, @mikachan! |
What?
@Mamaduka had a great idea to add an ESLint rule to help prevent translation functions from being used in save files for blocks.
This PR adds a new ESLint rule
@wordpress/no-i18n-in-savethat prevents the use of translation functions (__,_x,_n,_nx) in block save functions and save.js files.Why?
Using translation functions in save files often causes the block to break, as any translated text can make the block's HTML different to what is generated by the save function.
@t-hamano has some helpful docs around this here:
How?
Adds a new ESLint rule that will hopefully prevent translation functions from being added to block save functions. This rule should catch:
__,_x,_n,_nx) in any function named saveTesting Instructions
__()) to the fileThese tests should still pass:
npm run test:unit -- packages/eslint-plugin/rules/__tests__/no-i18n-in-save.js