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

chore: Don't allow string literals in IDs #10260

Merged
merged 4 commits into from Oct 3, 2018

Conversation

Projects
None yet
2 participants
@tofumatt
Member

tofumatt commented Sep 30, 2018

This adds a rule about hard-coding IDs to our eslint rules. We may also want to add a rule around Math.random() because we don't use it anywhere and it's a warning sign to someone trying to create UUIDs.

@tofumatt tofumatt requested review from aduth and WordPress/gutenberg-core Sep 30, 2018

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 2, 2018

Member

I updated this to ignore instances where literal IDs are fine (it's largely in test code, and for the DotTip component which uses a literal ID).

I feel strange disabling all of the no-restricted-syntax rules to ignore when we should have literal IDs but I don't think there's a better way. At this point I'm indifferent on this approach. Maybe it's helpful, or maybe it's too complicated and weird to be useful. 🤷‍♂️

Member

tofumatt commented Oct 2, 2018

I updated this to ignore instances where literal IDs are fine (it's largely in test code, and for the DotTip component which uses a literal ID).

I feel strange disabling all of the no-restricted-syntax rules to ignore when we should have literal IDs but I don't think there's a better way. At this point I'm indifferent on this approach. Maybe it's helpful, or maybe it's too complicated and weird to be useful. 🤷‍♂️

@aduth

aduth approved these changes Oct 3, 2018

// Discourage the usage of `Math.random()` as it's a code smell
// for UUID generation, for which we already have a higher-order
// component: `withInstanceId`.
selector: 'CallExpression[callee.object.name="Math"][callee.property.name="random"]',

This comment has been minimized.

@aduth

aduth Oct 3, 2018

Member

Might we want to contextualize this? To id?

https://astexplorer.net/#/gist/4f09c71cd79ebaca02a540aa8c2c4f9b/a3d4f17bf669d19d38b50a06dd1d50d8a9526360

Or is it intentional to cover more usage than just in id?

@aduth

aduth Oct 3, 2018

Member

Might we want to contextualize this? To id?

https://astexplorer.net/#/gist/4f09c71cd79ebaca02a540aa8c2c4f9b/a3d4f17bf669d19d38b50a06dd1d50d8a9526360

Or is it intentional to cover more usage than just in id?

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018

Member

It's intentional because we don't use it anywhere in the code base at the moment, and in the case of the Datepicker PR (#7621), it wasn't directly assigned to the id prop. It was assigned to a state variable and then used later, so I'm not sure it'd have been caught.

When do we ever really want to be using Math.random() anyhow? 😉

@tofumatt

tofumatt Oct 3, 2018

Member

It's intentional because we don't use it anywhere in the code base at the moment, and in the case of the Datepicker PR (#7621), it wasn't directly assigned to the id prop. It was assigned to a state variable and then used later, so I'm not sure it'd have been caught.

When do we ever really want to be using Math.random() anyhow? 😉

{
// The <DotTip> component uses the `id` prop for something that does not require an
// instanceId; maybe we should change its key.
selector: 'JSXOpeningElement[name.name!="DotTip"] JSXAttribute[name.name="id"][value.type="Literal"]',

This comment has been minimized.

@aduth

aduth Oct 3, 2018

Member

Not sure how I feel about the exception here. Conversely, it would not be good to push this on to the responsibility of the developer rendering DotTip. I think I'd be okay with this as-is, but if we get to a point where we want these to become more of a generalized style standard, we might want to reevaluate how we approach this.

@aduth

aduth Oct 3, 2018

Member

Not sure how I feel about the exception here. Conversely, it would not be good to push this on to the responsibility of the developer rendering DotTip. I think I'd be okay with this as-is, but if we get to a point where we want these to become more of a generalized style standard, we might want to reevaluate how we approach this.

This comment has been minimized.

@aduth

aduth Oct 3, 2018

Member

Another idea, though imperfect itself, could be to detect whether the prop on which the id is assigned is a built-in DOM element (lowercase) or custom component. Not sure this is something we can do with the selector system (vs. a more formal ESLint plugin), and it won't catch cases where in a custom component we pass through props to a DOM element (<Button id="" />), so okay to punt for later consideration.

@aduth

aduth Oct 3, 2018

Member

Another idea, though imperfect itself, could be to detect whether the prop on which the id is assigned is a built-in DOM element (lowercase) or custom component. Not sure this is something we can do with the selector system (vs. a more formal ESLint plugin), and it won't catch cases where in a custom component we pass through props to a DOM element (<Button id="" />), so okay to punt for later consideration.

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018

Member

I definitely wasn't a fan of this when I wrote it, and actually looking at it again I think it'd be better to // eslint-disable-next-line for DotTip's id prop for now, and look at changing DotTip's id prop to something else.

@tofumatt

tofumatt Oct 3, 2018

Member

I definitely wasn't a fan of this when I wrote it, and actually looking at it again I think it'd be better to // eslint-disable-next-line for DotTip's id prop for now, and look at changing DotTip's id prop to something else.

This comment has been minimized.

@tofumatt

tofumatt Oct 3, 2018

Member

Actually, yeah, it's worse to require any usage of DotTip to ignore an eslint rule. Even though it's only four instances, it's silly; I've filed #10305

@tofumatt

tofumatt Oct 3, 2018

Member

Actually, yeah, it's worse to require any usage of DotTip to ignore an eslint rule. Even though it's only four instances, it's silly; I've filed #10305

@tofumatt tofumatt added this to the 4.0 milestone Oct 3, 2018

@tofumatt tofumatt merged commit c178f1d into master Oct 3, 2018

2 checks passed

codecov/project 49.43% (+0.85%) compared to 0fe7d08
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the chore/with-instance-id branch Oct 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment