Add aphrodite-add-style-variable-name lint rule #1091

beaesguerra · 2024-12-11T00:36:43Z

Summary:

Adding a custom lint rule to ensure consistent naming for when we use addStyle for aphrodite styling. Variable naming should be in the format StyledTag.

By having predictable names for styled elements, we are able to use component mapping more effectively for eslint-plugin-jsx-a11y so it can use the underlying HTML for accessibility checks.

Issue: FEI-5952
Related ADR: ADR-781

Test plan:

Tests pass
Linting works in the demo project (will need some help with this!)

changeset-bot · 2024-12-11T00:36:46Z

🦋 Changeset detected

Latest commit: 638d66e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages

Name	Type
@khanacademy/eslint-config	Major
@khanacademy/eslint-plugin	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

beaesguerra · 2024-12-11T00:41:49Z

packages/eslint-plugin-khan/README.md

+## Creating a new lint rule
+
+Here are some helpful resources for setting up a new lint rule:
+
+- [TypeScript Eslint custom rules](https://typescript-eslint.io/developers/custom-rules/)
+- [AST Explorer](https://astexplorer.net/): A tool for showing what the abstract syntax tree (AST) looks like based on code
+- [ESTree Spec](https://github.com/estree/estree/tree/master): The spec for learning more about the AST node types


Let me know if there are other resources to include in this list! This is based on some resources Kevin pointed me to!

Good idea. Thanks for adding these links.

beaesguerra · 2024-12-11T00:44:24Z

packages/eslint-plugin-khan/docs/aphrodite-add-style-variable-name.md

@@ -0,0 +1,35 @@
+# Naming convention for addStyle variable (aphrodite-add-style-variable-name)


Let me know if there are certain conventions around custom lint rules! I tried to follow similar naming and documentation structure as our other custom lint rules

Thanks for following the existing patterns. LGTM :)

beaesguerra · 2024-12-11T00:46:52Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+    defaultOptions: [],
+    create(context) {
+        return {
+            VariableDeclarator(node: TSESTree.VariableDeclarator) {


chatgpt helped a lot with this 😄 I verified the structure with the AST explorer as well!

beaesguerra · 2024-12-11T00:59:01Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                                data: {
+                                    variableName,
+                                    tagName,
+                                    expectedName,


One thing to note is that it doesn't have an auto-fix option. We could potentially provide one since we have the expected name though. Auto-fixing existing places could be tricky I think since renaming a variable would mean we'd also need to update other places it is used. Let me know though if there's another way to approach this though!

It's probably good enough for now to ship this without the autofix. addStyle(" appears 127 times across 91 files. Some of those will be invalid cases with this rule but fixing them by hand shouldn't take too long. If you want to add autofixing in the future let me know and we can pair on it.

beaesguerra · 2024-12-11T01:05:55Z

packages/eslint-plugin-khan/demo/src/foo.tsx

+    // Placeholder example for calling addStyle for aphrodite
+}
+
+const div = addStyle("div");


I updated the demo project to include a linting error intentionally. Looks like it's working in CI! Some thoughts/questions:

I'm not sure why it shows the errors twice in the PR (same with the other errors)

Should CI be running linting in the demo project since we expect it to fail?

I followed the steps in the Demo readme locally and was running into an error. The linting error also wasn't being shown in VSCode. The error:

Oops! Something went wrong! :( ESLint: 8.40.0 TypeError: Failed to load plugin '@khanacademy' declared in '.eslintrc.js': Class extends value undefined is not a constructor or null

@kevinb-khan Have you come across this before?

Try updating eslint to 8.55.0.

Circling back so I remember what we did:

We updated eslint versions

We updated the demo project to use the released eslint-plugin-khan package. We'll enable the new addStyle rule in the demo project once these changes are released

We ignored the demo project from lint checks so that CI can pass (it is expected that the demo project has lint errors so we can test that rules are working as expected!)

kevinb-khan

LGTM 🎉 Thanks for adding this rule. I've left a few style suggestions in the comments.

kevinb-khan · 2024-12-12T18:27:23Z

packages/eslint-plugin-khan/docs/aphrodite-add-style-variable-name.md

+
+The variable name when using `addStyle` should be the same as the tag argument in the format `StyledTag`.
+
+This is useful so that Aphrodite styled elements can be mapped to HTML elements for static code analysis. For example, if the `addStyle` variables are consistently named, we are able to provide custom component mapping to `eslint-plugin-jsx-a11y` so that it can identify linting issues based on the underlying HTML tag.


Could you add a link to the docs for that rule?

kevinb-khan · 2024-12-12T18:27:53Z

packages/eslint-plugin-khan/README.md

+## Creating a new lint rule
+
+Here are some helpful resources for setting up a new lint rule:
+
+- [TypeScript Eslint custom rules](https://typescript-eslint.io/developers/custom-rules/)
+- [AST Explorer](https://astexplorer.net/): A tool for showing what the abstract syntax tree (AST) looks like based on code
+- [ESTree Spec](https://github.com/estree/estree/tree/master): The spec for learning more about the AST node types


Good idea. Thanks for adding these links.

kevinb-khan · 2024-12-12T19:39:12Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                    node.init &&
+                    node.init.type === "CallExpression" &&
+                    node.init.callee.type === "Identifier" &&
+                    node.init.callee.name === "addStyle"


style suggestion: some of our other code compares .type fields to things like AST_NODE_TYPES.Identifier. AST_NODE_TYPES can be imported from "@typescript-eslint/utils".

style suggestion: this could be one or early returns to avoid excessive indentation, e.g.

if (node?.init.type !== "CallExpression") { return; } if (node.init.callee.type !== "Identifier" || node.init.callee.name !== "addStyle) { return; } ...

Thanks for the feedback Kevin! I refactored the function to favour early returns. It is easier to read now 😄

kevinb-khan · 2024-12-12T19:40:26Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                    // Get variable name for the addStyle return value
+                    const variableName =
+                        node.id.type === "Identifier" ? node.id.name : null;


style suggestion: this check could be an early return

kevinb-khan · 2024-12-12T19:42:48Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                    const firstArg = node.init.arguments[0];
+                    if (
+                        firstArg &&
+                        firstArg.type === "Literal" &&
+                        typeof firstArg.value === "string"
+                    ) {


style suggestion: this check could be an early return

kevinb-khan · 2024-12-12T19:44:13Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                        const expectedName = `Styled${tagName
+                            .charAt(0)
+                            .toUpperCase()}${tagName.slice(1)}`;


style nit: you may want to extract the part that does the capitalization into a helper function call capitalizeString. This will make it easier for people to grok what's going on here.

Good idea! I moved it to a toPascalCase function to handle the custom html element use case you mentioned!

kevinb-khan · 2024-12-12T19:46:15Z

packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

+                                data: {
+                                    variableName,
+                                    tagName,
+                                    expectedName,


It's probably good enough for now to ship this without the autofix. addStyle(" appears 127 times across 91 files. Some of those will be invalid cases with this rule but fixing them by hand shouldn't take too long. If you want to add autofixing in the future let me know and we can pair on it.

kevinb-khan · 2024-12-12T19:50:51Z

packages/eslint-plugin-khan/test/rules/aphrodite-add-style-variable-name.test.ts

+        {
+            code: `const StyledSup = addStyle("sup")`,
+        },


suggestion: It might be worth handling things like addStyle("foo-bar") to enable handling of custom HTML components. I know we don't use any right now, but we might someday in the future or other people/orgs might use this eslint rule (it is open source). Being pro-active is pretty low cost in this situation.

Good idea! Instead of capitalizing the tag name for the expected variable name, I updated it to turn the string into PascalCase. I added a test for this (and other types of text casing like foo_bar or FooBar just in case!)

Awesome. Thank you!

kevinb-khan · 2024-12-12T19:54:21Z

packages/eslint-plugin-khan/test/rules/aphrodite-add-style-variable-name.test.ts

+        {
+            code: `const StyledDiv = addStyle("div")`,
+        },


Does this rule ignore things like const MyCompWithStyle = addStyle(MyComp)?

Good question, yes! It will return early if the addStyle argument is not a string:

wonder-stuff/packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts

Lines 79 to 85 in fc49725

// Return early if the argument is not a string literal

if (

firstArg?.type !== AST_NODE_TYPES.Literal ||

typeof firstArg.value !== "string"

) {

return;

}

I updated the tests and docs to explain that the naming convention only applies to string arguments for html tags!

(I didn't know addStyle could be used with components, thank you for bringing this up!)

I don't see any uses of it in our code. I usually tell people to manually add a style prop. Since that's what all of our code does I guess people just copy/pasting that now which is good.

…on't automatically lint files in the demo folder

github-actions · 2024-12-12T21:47:57Z

Size Change: 0 B

Total Size: 4.63 kB

ℹ️ View Unchanged

Filename	Size
`packages/wonder-stuff-core/dist/browser/es/index.js`	1.85 kB
`packages/wonder-stuff-sentry/dist/browser/es/index.js`	1.65 kB
`packages/wonder-stuff-testing/dist/browser/es/index.js`	1.12 kB

_{compressed-size-action}

codecov · 2024-12-12T22:01:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (4cba5ba) to head (638d66e).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1091   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          97       97           
  Lines        1392     1392           
  Branches      358      351    -7     
=======================================
  Hits         1390     1390           
  Misses          1        1           
  Partials        1        1

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cba5ba...638d66e. Read the comment docs.

… doesn't enforce the linting convention

beaesguerra · 2024-12-12T23:33:27Z

packages/eslint-plugin-khan/demo/.eslintrc.js

 module.exports = {
+    root: true,


We set this so that the demo eslint config isn't merged with parent eslint config files!

beaesguerra · 2024-12-12T23:40:39Z

packages/eslint-plugin-khan/demo/src/foo.tsx

+    // Placeholder example for calling addStyle for aphrodite
+}
+
+const div = addStyle("div");


Circling back so I remember what we did:

We updated eslint versions

We updated the demo project to use the released eslint-plugin-khan package. We'll enable the new addStyle rule in the demo project once these changes are released

We ignored the demo project from lint checks so that CI can pass (it is expected that the demo project has lint errors so we can test that rules are working as expected!)

…rent text casing

khan-actions-bot · 2024-12-13T00:01:04Z

Gerald

Required Reviewers

@Khan/frontend-infra for changes to .eslintignore, lint_ignore.txt, package.json, yarn.lock, .changeset/brave-scissors-repair.md, .changeset/dirty-islands-bake.md, packages/eslint-config-khan/package.json, packages/eslint-plugin-khan/README.md, packages/eslint-plugin-khan/package.json, packages/eslint-plugin-khan/demo/.eslintrc.js, packages/eslint-plugin-khan/demo/package.json, packages/eslint-plugin-khan/demo/yarn.lock, packages/eslint-plugin-khan/demo/src/foo.tsx, packages/eslint-plugin-khan/src/rules/aphrodite-add-style-variable-name.ts, packages/eslint-plugin-khan/src/rules/index.ts, packages/eslint-plugin-khan/test/rules/aphrodite-add-style-variable-name.test.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

kevinb-khan

Thanks for all of the changes. This looks great. 🚀

add readme docs for resources to creating a new lint rule

717bfea

beaesguerra self-assigned this Dec 11, 2024

beaesguerra force-pushed the lint-rule-for-aphrodite branch from 55000ef to 770fc81 Compare December 11, 2024 00:43

beaesguerra added 2 commits December 10, 2024 17:55

set up aphrodite add style lint rule

81518c4

trying to set up add style line rule in the demo project

6811682

beaesguerra force-pushed the lint-rule-for-aphrodite branch from 770fc81 to 6811682 Compare December 11, 2024 00:55

beaesguerra commented Dec 11, 2024

View reviewed changes

add changeset

f67f89c

kevinb-khan approved these changes Dec 12, 2024

View reviewed changes

beaesguerra and others added 4 commits December 12, 2024 13:23

update eslint to 8.55.0

a2a84c2

don't allow other .eslintrc files to cascade into demo's .eslintrc, d…

707a8c7

…on't automatically lint files in the demo folder

try to make khan-linter happy

1b528f0

use a real version for the eslint plugin dep in the demo folder

2fc5540

beaesguerra added 6 commits December 12, 2024 15:27

add link to a11y component mapping eslint plugin

c22082d

use AST_NODE_TYPES enum for node types

80eeb0a

Favour early returns instead of nested if statements

61ab9e0

Handle custom html components

1ac9cd8

Update tests and docs to mention that using a component with addStyle…

e674d22

… doesn't enforce the linting convention

add changeset for version bump

fc49725

beaesguerra commented Dec 12, 2024

View reviewed changes

check if tag is already using PascalCase and add more tests for diffe…

638d66e

…rent text casing

beaesguerra marked this pull request as ready for review December 13, 2024 00:00

beaesguerra requested a review from kevinb-khan December 13, 2024 00:00

beaesguerra requested a review from a team December 13, 2024 00:09

kevinb-khan approved these changes Dec 13, 2024

View reviewed changes

beaesguerra merged commit 3d8fea4 into main Dec 13, 2024
9 checks passed

beaesguerra deleted the lint-rule-for-aphrodite branch December 13, 2024 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add aphrodite-add-style-variable-name lint rule #1091

Add aphrodite-add-style-variable-name lint rule #1091

beaesguerra commented Dec 11, 2024 •

edited

Loading

changeset-bot bot commented Dec 11, 2024 •

edited

Loading

beaesguerra Dec 11, 2024 •

edited

Loading

kevinb-khan Dec 12, 2024

beaesguerra Dec 11, 2024

kevinb-khan Dec 13, 2024

beaesguerra Dec 11, 2024

beaesguerra Dec 11, 2024

kevinb-khan Dec 12, 2024

beaesguerra Dec 11, 2024 •

edited

Loading

kevinb-khan Dec 12, 2024

beaesguerra Dec 12, 2024

kevinb-khan left a comment

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

beaesguerra Dec 12, 2024

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

beaesguerra Dec 12, 2024

kevinb-khan Dec 12, 2024

kevinb-khan Dec 12, 2024

beaesguerra Dec 12, 2024 •

edited

Loading

kevinb-khan Dec 13, 2024

kevinb-khan Dec 12, 2024

beaesguerra Dec 12, 2024 •

edited

Loading

kevinb-khan Dec 13, 2024

github-actions bot commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading

beaesguerra Dec 12, 2024

beaesguerra Dec 12, 2024

khan-actions-bot commented Dec 13, 2024

kevinb-khan left a comment

		@@ -0,0 +1,35 @@
		# Naming convention for addStyle variable (aphrodite-add-style-variable-name)


		The variable name when using `addStyle` should be the same as the tag argument in the format `StyledTag`.

		This is useful so that Aphrodite styled elements can be mapped to HTML elements for static code analysis. For example, if the `addStyle` variables are consistently named, we are able to provide custom component mapping to `eslint-plugin-jsx-a11y` so that it can identify linting issues based on the underlying HTML tag.

	// Return early if the argument is not a string literal
	if (
	firstArg?.type !== AST_NODE_TYPES.Literal \|\|
	typeof firstArg.value !== "string"
	) {
	return;
	}

Add aphrodite-add-style-variable-name lint rule #1091

Add aphrodite-add-style-variable-name lint rule #1091

Conversation

beaesguerra commented Dec 11, 2024 • edited Loading

Summary:

Test plan:

changeset-bot bot commented Dec 11, 2024 • edited Loading

🦋 Changeset detected

beaesguerra Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaesguerra Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinb-khan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaesguerra Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beaesguerra Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 12, 2024 • edited Loading

codecov bot commented Dec 12, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khan-actions-bot commented Dec 13, 2024

Gerald

kevinb-khan left a comment

Choose a reason for hiding this comment

beaesguerra commented Dec 11, 2024 •

edited

Loading

changeset-bot bot commented Dec 11, 2024 •

edited

Loading

beaesguerra Dec 11, 2024 •

edited

Loading

beaesguerra Dec 11, 2024 •

edited

Loading

beaesguerra Dec 12, 2024 •

edited

Loading

beaesguerra Dec 12, 2024 •

edited

Loading

github-actions bot commented Dec 12, 2024 •

edited

Loading

codecov bot commented Dec 12, 2024 •

edited

Loading