Add require-autocomplete lint rule #251

andrewmcgov · 2021-05-21T19:59:50Z

Description

Adds an eslint rule that tries to ensure we include an autoComplete attribute on input. Part of the Autocomplete project.

Valid input example:

<input type="email" label="Email" autoComplete="email" />

Invalid input example:

<input type="email" label="Email" />

// Input elements of type 'email' must have an autoComplete attribute eslint(@shopify/react-require-autocomplete)

This rule catches most issues, but there are some circumstances where props passed into an input or TextField may be defined in a different module (passed as props to a parent for example) making it difficult to know for sure if an autoComplete value was passed or not. In these cases we do not report an error.

Type of change

Patch: Bug (non-breaking change which fixes an issue)
Minor: New feature (non-breaking change which adds functionality)
Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish a new version for these changes)

andrewmcgov · 2021-06-15T19:49:37Z

packages/eslint-plugin/lib/rules/react-require-autocomplete.js

+  'url',
+  'week',
+];
+const autocompleteElementNames = ['input', 'TextField'];


If we have other components that some libraries may want the rule applied to, I think we could make this a setting.

We probably should make it a setting, for example on web I can think of the SimpleDateField component that could benefit from it 🤔

Ha, I was looking into whether we have the autocomplete-valid rule enabled and it seems like we apply it on the <Select> component too! We should probably do the same here.

andrewmcgov · 2021-06-15T19:57:17Z

packages/eslint-plugin/lib/config/rules/shopify.js

@@ -45,6 +45,8 @@ module.exports = {
  '@shopify/react-no-multiple-render-methods': 'off',
  // Prefer all non-React-specific members be marked private in React class components.
  '@shopify/react-prefer-private-members': 'off',
+  // Require input elements to have autocomplete values
+  '@shopify/react-require-autocomplete': 'off',


Does this make the rule disabled by default, allowing apps to opt in on their own time?

Tom-Bonnike

Super cool, good work! Your commit still mentions it‘s a WIP you may want to amend that 😜

Tom-Bonnike · 2021-06-17T13:56:46Z

packages/eslint-plugin/lib/rules/react-require-autocomplete.js

+  'url',
+  'week',
+];
+const autocompleteElementNames = ['input', 'TextField'];


We probably should make it a setting, for example on web I can think of the SimpleDateField component that could benefit from it 🤔

Tom-Bonnike · 2021-06-17T14:00:17Z

packages/eslint-plugin/tests/lib/rules/react-require-autocomplete.test.js

+        return <TextField type="text" {...fieldProps} />;
+      }`,
+    },
+  ],


I think we could add two more test cases:

for an input type on which autoComplete is not valid (e.g. <input type="checkbox" />)

a test that ensures we don’t warn about components other than input and TextField

Tom-Bonnike · 2021-06-17T14:04:47Z

packages/eslint-plugin/lib/config/react.js

@@ -26,6 +26,7 @@ module.exports = {
      '@shopify/react-initialize-state': 'error',
      '@shopify/react-no-multiple-render-methods': 'error',
      '@shopify/react-prefer-private-members': 'error',
+      '@shopify/react-require-autocomplete': 'error',


I’m guessing you thought this through already but… could there be any case where we wouldn’t want an autoComplete on one of these input types? Asking because maybe it should simply be a warn.

eslint when ran via sewing-kit runs with maxWarnings=0 so errors and warnings are functionally identical - any of either will cause CI to fail. We tend to not use warnings at all because you can't tell the difference between a warning that is left that way for a reason and a warning that is an oversight that should be fixed. Using an eslint-disable-next-line comment is a clearer way to say "this is a violation but we know better"

Tom-Bonnike · 2021-06-17T14:09:14Z

packages/eslint-plugin/lib/rules/react-require-autocomplete.js

+  }),
+};
+
+function findAttrValue(attributes, name, context) {


Interesting, I’d recommend using a module like jsx-ast-utils so we don’t have to implement these utils ourselves?

vsumner

Great work! Thank you for providing tests and the detailed documentation of this new rule.

I tested this against Shopify/web and because the rule is set to error for react it is indeed on by default.

There were 54 errors and they seem to be legitimate.

error: Input elements of type 'text' must have an autoComplete attribute (@shopify/react-require-autocomplete) at app/components/SkeletonIndex/SkeletonIndex.tsx:34:15:
  32 |             <SkeletonTabs />
  33 |             <Card.Section>
> 34 |               <TextField
     |               ^
  35 |                 connectedLeft={filterButton}
  36 |                 label=""
  37 |                 labelHidden


error: Input elements of type 'text' must have an autoComplete attribute (@shopify/react-require-autocomplete) at app/components/SsnField/SsnField.tsx:41:5:
  39 |
  40 |   return (
> 41 |     <TextField
     |     ^
  42 |       value={format(value)}
  43 |       label={label}
  44 |       helpText={helpText}

I think minor is safe since this is a new feature but not a breaking change.

andrewmcgov · 2021-06-21T18:22:51Z

Thanks @vsumner and @Tom-Bonnike!

I've update the PR with your feedback and added a changelog entry.

We're going to leave this as error by default and let individual repos turn it off if they want to.
Thanks to Tom's suggestion I used a couple helper functions from jsx-ast-utils. This lead to me being able to delete most of the code I had written for the rule 😅. I ran the same test that Victor ran on web and got 53 errors instead of 54, which is fine.
I added a setting to the rule for repo's to add elements to have the rule applied to. For example to test in web I added the following to the .eslintrc.js file:

'@shopify/react-require-autocomplete': [
  'error',
  {
    inputComponents: ['TextField'],
  },
],

Let me know if I am missing anything else!

BPScott

Code is mostly looking good. Added a few questions inline, mainly around capitalisation of attributes - i thought they'd always be lowercase.

BPScott · 2021-06-21T20:18:52Z

packages/eslint-plugin/docs/rules/react-require-autocomplete.md

@@ -0,0 +1,31 @@
+# Require inputs have have autocomplete attributes when necessary
+
+The HTML `autoComplete` helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.


Suggested change

The HTML `autoComplete` helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.

The HTML `autocomplete` attribute helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.

I'm using autoComplete with an uppercase c here because the rule is targeted towards React, and that the supported attribute has the uppercase letter: https://reactjs.org/docs/dom-elements.html#all-supported-html-attributes.

Do you think I should change the value here in the docs?

I've adjusted the readme a bit. I start with autocomplete and then state that in React the attribute is written in camelCase. Then I use autoComplete from that point on.

BPScott · 2021-06-21T20:19:54Z

packages/eslint-plugin/lib/config/rules/shopify.js

@@ -45,6 +45,8 @@ module.exports = {
  '@shopify/react-no-multiple-render-methods': 'off',
  // Prefer all non-React-specific members be marked private in React class components.
  '@shopify/react-prefer-private-members': 'off',
+  // Require input elements to have autocomplete values
+  '@shopify/react-require-autocomplete': 'off',


BPScott · 2021-06-21T20:20:50Z

packages/eslint-plugin/lib/rules/react-require-autocomplete.js

+        return;
+      }
+
+      const hasAutoCompleteProp = hasProp(node.attributes, 'autoComplete');


Should attributes be lowercase rather than camel case? autocomplete rather than autoComplete?

BPScott · 2021-06-21T20:23:02Z

packages/eslint-plugin/lib/utilities/index.js

@@ -147,6 +147,7 @@ module.exports = {
  polarisComponentFromJSX,
  getRootObject,
  docsUrl,
+  findDefinition,


Where is this used, can it be removed? It does not seem to be a top-level function in this file so iI'd be suprised if this even works.

I was using this function in a previous version of the lint rule and forgot to remove this after the refactor. Good catch!

BPScott

Nice! Thanks for doing this, and thanks for teaching me autoComplete is one of those react uses different conventions edge-cases.

One nitpick about changelog formatting to fixup then you can go squash and merge!

BPScott · 2021-06-22T17:01:02Z

packages/eslint-plugin/CHANGELOG.md

@@ -7,6 +7,8 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 <!-- ## Unreleased -->


Uncomment the heading, and add a subheading now there is changelog content for a new release

Suggested change



## Unreleased

### Changed

andrewmcgov force-pushed the add-autocomplete-lint-rule branch 4 times, most recently from d14ca8f to 0ffd508 Compare June 4, 2021 20:31

andrewmcgov force-pushed the add-autocomplete-lint-rule branch 2 times, most recently from cb0f08a to 9852a93 Compare June 15, 2021 19:46

andrewmcgov commented Jun 15, 2021

View reviewed changes

andrewmcgov marked this pull request as ready for review June 15, 2021 19:57

andrewmcgov changed the title ~~WIP - Add require-autocomplete lint rule~~ Add require-autocomplete lint rule Jun 15, 2021

Tom-Bonnike reviewed Jun 17, 2021

View reviewed changes

vsumner approved these changes Jun 17, 2021

View reviewed changes

andrewmcgov force-pushed the add-autocomplete-lint-rule branch from 9852a93 to e605405 Compare June 21, 2021 17:41

BPScott reviewed Jun 21, 2021

View reviewed changes

andrewmcgov force-pushed the add-autocomplete-lint-rule branch from e605405 to 1303d63 Compare June 21, 2021 20:54

BPScott mentioned this pull request Jun 22, 2021

[Eslint] jsx-ally | Removing jsx autocomplete valid rule #264

Merged

4 tasks

BPScott approved these changes Jun 22, 2021

View reviewed changes

Add react-require-autocomplete lint rule

8556bfc

andrewmcgov force-pushed the add-autocomplete-lint-rule branch from 1303d63 to 8556bfc Compare June 22, 2021 17:40

andrewmcgov merged commit bdaccc1 into main Jun 22, 2021

andrewmcgov deleted the add-autocomplete-lint-rule branch June 22, 2021 17:48

shopify-shipit bot temporarily deployed to production June 30, 2021 18:51 Inactive

renovate bot mentioned this pull request Jun 30, 2021

chore(deps): update dependency @shopify/eslint-plugin to v40.3.0 bo2kshelf/eslint-plugin#24

Open

1 task

shopify-shipit bot had a problem deploying to loose-assumptions July 14, 2021 18:35 Failure

shopify-shipit bot had a problem deploying to loose-assumptions July 14, 2021 19:11 Failure

shopify-shipit bot deployed to loose-assumptions July 14, 2021 19:32 Active

renovate bot mentioned this pull request Sep 13, 2021

Update dependency @shopify/eslint-plugin to v40.4.0 PascalMayr/rebelz_upsell#55

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add require-autocomplete lint rule #251

Add require-autocomplete lint rule #251

andrewmcgov commented May 21, 2021 •

edited

Loading

andrewmcgov Jun 15, 2021

Tom-Bonnike Jun 17, 2021

Tom-Bonnike Jun 17, 2021 •

edited

Loading

andrewmcgov Jun 15, 2021

BPScott Jun 21, 2021

Tom-Bonnike left a comment

Tom-Bonnike Jun 17, 2021

Tom-Bonnike Jun 17, 2021 •

edited

Loading

Tom-Bonnike Jun 17, 2021

BPScott Jun 22, 2021

Tom-Bonnike Jun 17, 2021

vsumner left a comment

andrewmcgov commented Jun 21, 2021

BPScott left a comment

BPScott Jun 21, 2021

andrewmcgov Jun 21, 2021

andrewmcgov Jun 21, 2021

BPScott Jun 21, 2021

BPScott Jun 21, 2021

BPScott Jun 21, 2021

andrewmcgov Jun 21, 2021

BPScott left a comment

BPScott Jun 22, 2021

		@@ -0,0 +1,31 @@
		# Require inputs have have autocomplete attributes when necessary

		The HTML `autoComplete` helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.

	The HTML `autoComplete` helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.
	The HTML `autocomplete` attribute helps users to complete filling in forms by using data stored in the browser. This is particularly useful for people with motor disabilities or cognitive impairment who may have difficulties filling out forms online.

		@@ -7,6 +7,8 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

		<!-- ## Unreleased -->

Add require-autocomplete lint rule #251

Add require-autocomplete lint rule #251

Conversation

andrewmcgov commented May 21, 2021 • edited Loading

Description

Type of change

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tom-Bonnike Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tom-Bonnike left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tom-Bonnike Jun 17, 2021 • 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

vsumner left a comment

Choose a reason for hiding this comment

andrewmcgov commented Jun 21, 2021

BPScott 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

BPScott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewmcgov commented May 21, 2021 •

edited

Loading

Tom-Bonnike Jun 17, 2021 •

edited

Loading

Tom-Bonnike Jun 17, 2021 •

edited

Loading