Skip to content

Commit

Permalink
Introduce custom rules for i18n imports (#67)
Browse files Browse the repository at this point in the history
Before:

- `import i18n from discourse-common/helpers/i18n;` -> `i18n()`
- `import I18n from "discourse-i18n";` -> `I18n.t()`
- `import I18n from "i18n";` -> `I18n.t()`

After:

- `import { i18n } from "discourse-i18n";` -> `i18n()`

These rules remain disabled by default. We can enable them by default once discourse/discourse@d606ac3d8e is widely available.
  • Loading branch information
davidtaylorhq authored Nov 19, 2024
1 parent de519b2 commit 0d58b40
Show file tree
Hide file tree
Showing 11 changed files with 792 additions and 12 deletions.
64 changes: 64 additions & 0 deletions lint-configs/eslint-rules/i18n-import-location.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import { fixImport } from "./utils/fix-import.mjs";

export default {
meta: {
type: "suggestion",
docs: {
description:
"disallow imports from 'i18n' and replace with 'discourse-i18n'",
category: "Best Practices",
recommended: false,
},
fixable: "code",
schema: [], // no options
},
create(context) {
return {
ImportDeclaration(node) {
if (node.source.value.toLowerCase() === "i18n") {
context.report({
node,
message:
"Import from 'i18n' is not allowed. Use 'discourse-i18n' instead.",
fix(fixer) {
return fixer.replaceText(node.source, "'discourse-i18n'");
},
});
}

if (
node.source.value.toLowerCase() === "discourse-common/helpers/i18n"
) {
context.report({
node,
message:
"Import from 'discourse-common/helpers/i18n' is not allowed. Use 'discourse-i18n' instead.",
fix(fixer) {
const existingImport = context
.getSourceCode()
.ast.body.find(
(n) =>
n.type === "ImportDeclaration" &&
n.source.value === "discourse-i18n"
);

if (existingImport) {
return [
fixer.remove(node),
fixImport(fixer, existingImport, {
namedImportsToAdd: ["i18n"],
}),
];
} else {
return fixer.replaceText(
node,
`import { i18n } from 'discourse-i18n';`
);
}
},
});
}
},
};
},
};
91 changes: 91 additions & 0 deletions lint-configs/eslint-rules/i18n-t.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
import { fixImport } from "./utils/fix-import.mjs";

export default {
meta: {
type: "suggestion",
docs: {
description: "Use i18n(...) instead of 'I18n.t(...)'.",
category: "Best Practices",
recommended: false,
},
fixable: "code",
schema: [], // no options
},
create(context) {
const sourceCode = context.sourceCode ?? context.getSourceCode();
let alreadyFixedImport = false;

return {
MemberExpression(node) {
const isI18nT =
node.object.name === "I18n" && node.property.name === "t";
if (!isI18nT) {
return;
}

let scope = sourceCode.getScope(node);
let variable;
while (scope && !variable) {
variable = scope.variables.find((v) => v.name === "I18n");
scope = scope.upper;
}

if (!variable) {
return;
}

const i18nDefaultImport = variable.defs.find(
(d) =>
d.type === "ImportBinding" &&
d.node.type === "ImportDefaultSpecifier" &&
d.node.parent.source.value === "discourse-i18n"
);

if (!i18nDefaultImport) {
// I18n imported from elsewhere... weird!
return;
}

context.report({
node,
message: "Use 'i18n(...)' instead of 'I18n.t(...)'.",
fix(fixer) {
const fixes = [];

// Replace I18n.t with i18n
fixes.push(fixer.replaceText(node, `i18n`));

if (!alreadyFixedImport) {
const importDeclaration = i18nDefaultImport.node.parent;
const i18nSpecifier = importDeclaration.specifiers.find(
(specifier) =>
specifier.type === "ImportSpecifier" &&
specifier.imported.name === "i18n"
);

// Check if I18n is used elsewhere
const shouldRemoveDefaultImport = !variable.references.some(
(ref) =>
ref.identifier.parent.type !== "MemberExpression" ||
ref.identifier.parent.property.name !== "t"
);

if (!i18nSpecifier || shouldRemoveDefaultImport) {
fixes.push(
fixImport(fixer, importDeclaration, {
shouldHaveDefaultImport: !shouldRemoveDefaultImport,
namedImportsToAdd: ["i18n"],
})
);
}

alreadyFixedImport = true;
}

return fixes;
},
});
},
};
},
};
69 changes: 69 additions & 0 deletions lint-configs/eslint-rules/utils/fix-import.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* Fix an import declaration
*
* @param {ASTNode} importDeclarationNode - The AST node representing the import declaration.
* @param {Object} options - Options for modifying the import statement.
* @param {boolean} options.shouldHaveDefaultImport - Whether the import should include a default import.
* @param {string[]} options.namedImportsToAdd - Named imports to add to the import statement.
* @param {string[]} options.namedImportsToRemove - Named imports to remove from the import statement.
*/
export function fixImport(
fixer,
importDeclarationNode,
{ shouldHaveDefaultImport, namedImportsToAdd = [], namedImportsToRemove = [] }
) {
const existingSpecifiers = importDeclarationNode.specifiers;
const existingDefaultImport = existingSpecifiers.find(
(specifier) => specifier.type === "ImportDefaultSpecifier"
);

// Map existing named imports to their local names
const existingNamedImports = existingSpecifiers
.filter((specifier) => specifier.type === "ImportSpecifier")
.reduce((acc, specifier) => {
acc[specifier.imported.name] = specifier.local.name;
return acc;
}, {});

// Determine final default import
let finalDefaultImport;
if (shouldHaveDefaultImport === undefined) {
finalDefaultImport = existingDefaultImport
? existingDefaultImport.local.name
: null;
} else if (shouldHaveDefaultImport) {
finalDefaultImport = existingDefaultImport
? existingDefaultImport.local.name
: shouldHaveDefaultImport;
} else {
finalDefaultImport = null;
}

// Determine final named imports, preserving aliases
const finalNamedImports = Array.from(
new Set([
...Object.entries(existingNamedImports)
.filter(([imported]) => !namedImportsToRemove.includes(imported))
.map(([imported, local]) =>
imported === local ? imported : `${imported} as ${local}`
),
...namedImportsToAdd,
])
);

// Construct the new import statement
let newImportStatement = "import ";
if (finalDefaultImport) {
newImportStatement += `${finalDefaultImport}`;
if (finalNamedImports.length > 0) {
newImportStatement += ", ";
}
}
if (finalNamedImports.length > 0) {
newImportStatement += `{ ${finalNamedImports.join(", ")} }`;
}
newImportStatement += ` from '${importDeclarationNode.source.value}';`;

// Replace the entire import declaration
return fixer.replaceText(importDeclarationNode, newImportStatement);
}
12 changes: 12 additions & 0 deletions lint-configs/eslint.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import QUnitRecommended from "eslint-plugin-qunit/configs/recommended";
import SimpleImportSort from "eslint-plugin-simple-import-sort";
import SortClassMembers from "eslint-plugin-sort-class-members";
import globals from "globals";
import i18nImport from "./eslint-rules/i18n-import-location.mjs";
import i18nT from "./eslint-rules/i18n-t.mjs";

// Copied from "ember-template-imports/lib/utils"
const TEMPLATE_TAG_PLACEHOLDER = "__GLIMMER_TEMPLATE";
Expand Down Expand Up @@ -85,6 +87,12 @@ export default [
"decorator-position": DecoratorPosition,
"simple-import-sort": SimpleImportSort,
qunit: QUnitPlugin,
discourse: {
rules: {
"i18n-import-location": i18nImport,
"i18n-t": i18nT,
},
},
},
rules: {
"block-scoped-var": "error",
Expand Down Expand Up @@ -249,6 +257,10 @@ export default [
],
},
],
// TODO: enable by default once this commit is available widely
// https://github.com/discourse/discourse/commit/d606ac3d8e
// "discourse/i18n-import-location": ["error"],
// "discourse/i18n-t": ["error"],
},
},
{
Expand Down
3 changes: 3 additions & 0 deletions lint-configs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@
},
"./template-lint": {
"require": "./template-lint.config.cjs"
},
"./eslint-discourse/*": {
"require": "./eslint-discourse/*.cjs"
}
},
"scripts": {
Expand Down
Loading

0 comments on commit 0d58b40

Please sign in to comment.