Skip to content

Fix Typography lint catching non truthy props #1137

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jan 28, 2021
Merged

Fix Typography lint catching non truthy props #1137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0b85a8e
Fix Typography lint catching non truthy props
M-i-k-e-l Jan 14, 2021
72ec0c3
Check for relevant components instead of value
M-i-k-e-l Jan 26, 2021
fc5c00a
Merge branch 'master' into Infra/fix-typography-lint
ethanshar Jan 27, 2021
3d74107
Merge branch 'master' into Infra/fix-typography-lint
M-i-k-e-l Jan 27, 2021
283cd9a
Add comments
M-i-k-e-l Jan 27, 2021
ef30ba2
Merge branch 'master' into Infra/fix-typography-lint
M-i-k-e-l Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion eslint-rules/lib/rules/typography-deprecation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ module.exports = {
}

function testJSXAttribute(node) {
if (node && node.name) {
if (node && node.name && !node.value) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this check is because people use typography modifiers like this?

<Text bodyBold />

And the prop doesn't have an actual value (it behaves as boolean true) so you assume it's a modifier?

If that's right I think you might be missing something.
What if someone renders this

const useBodyBold = <some condition>;
<Text bodyBold={useBodyBold} />

In this case the value will exist and can be true or false.

Maybe the check should be more explicit and ignore boolean values

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it was the title case (which can be an actual title in some components).
That's a good point, I'll add a test and fix accordingly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell there's only the Identifier type (no boolean, string etc), so we can't tell what the value is unless we start to dig in deeper into the tree, I don't think it's worth it at this stage, WDYT?

For reference I'll leave two example node.value:
title (string):
{"type":"JSXExpressionContainer","start":155,"end":162,"loc":{"start":{"line":5,"column":27},"end":{"line":5,"column":34}},"expression":{"type":"Identifier","start":156,"end":161,"loc":{"start":{"line":5,"column":28},"end":{"line":5,"column":33},"identifierName":"title"},"name":"title","range":{"0":156,"1":161},"_babelType":"Identifier"},"range":{"0":155,"1":162},"_babelType":"JSXExpressionContainer"}
isItDeprecated (true):
{"type":"JSXExpressionContainer","start":103,"end":119,"loc":{"start":{"line":3,"column":29},"end":{"line":3,"column":45}},"expression":{"type":"Identifier","start":104,"end":118,"loc":{"start":{"line":3,"column":30},"end":{"line":3,"column":44},"identifierName":"isItDeprecated"},"name":"isItDeprecated","range":{"0":104,"1":118},"_babelType":"Identifier"},"range":{"0":103,"1":119},"_babelType":"JSXExpressionContainer"}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked in https://astexplorer.net
And inside JSXExpressionContainer you can find an expression that holds the value.
You can check typeof value to get the type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice site, thanks :)
Not sure how I can get the value though (again, without traversal).
Do you have a way to do that without traversal?
If not, I suggest we open a task and do that after the refactor.

Code examples:
1.
import React, {Component} from 'react';
import {Typography} from 'our-source';
import {List} from 'another-source';
export default class OurList extends Component {
render() {
const title = 'bla';
return (
<List.Item title={title}/>
)
}
}

  1. Same but with:
    const title = true;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll help because generally components that have Props that extend TextProps or TypographyModifiers do not have the title prop.
So, if they have title as a prop, it'll be the typography, and we don't need to check for a value.
I think that if a component will have two props with the same name (i.e. a prop and a modifier that have the same name), it'll be a bigger problem than the lint rules, so it should not happen.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good..
But we can still have this, no?

const useTitle = ....;
<Text title={useTitle}>My Text</Text>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but if the user has title it's already "bad", i.e. deprecated, no matter what the value is

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohhhh right right.
ok sounds good.
Let's do that - check on specific components

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
I've added some commented tests for stuff we don't yet support (renamed components - should be easily supported after the refactor, props of components - might be supported with Lidor's PR).

findAndReportDeprecation(node, node.name.name, true);
}
}
Expand Down
2 changes: 1 addition & 1 deletion eslint-rules/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-plugin-uilib",
"version": "1.0.27",
"version": "1.0.28",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that if we publish this version, it'll be used in production.
Are we ok with that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so (does hoping really hard helps? 🤞 ).
Do you have any reservations?

"description": "uilib set of eslint rules",
"keywords": [
"eslint",
Expand Down
7 changes: 7 additions & 0 deletions eslint-rules/tests/lib/rules/typography-deprecation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,13 @@ ruleTester.run('typography-deprecation', rule, {
{
options: options,
code: `${fullClassValidRenamed}`,
},
{
options: options,
code: `
${ourImport}
import {List} from 'another-source';
<List.Item title={'bla'} />`
}
],
invalid: [
Expand Down
5 changes: 5 additions & 0 deletions eslint-rules/tests/typography_deprecation.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,10 @@
"path": "Typography.deprecated",
"message": "Please use 'Typography.valid' instead (fix is available).",
"fix": "Typography.valid"
},
{
"path": "Typography.title",
"message": "Please use 'Typography.heading' instead (fix is available).",
"fix": "Typography.heading"
}
]