Fails at line 33's node.value.type check because value is undefined:

Sample crashy input:

class Foo extends React.PureComponent<ComposedProps, State> {
  state: State;

  constructor(props: ComposedProps) {
    super(props);

    this.state = {selectedSrc};
  }
}

cc @lemonmade

#93 (comment)

These scripts are really useful during dependency upgrades :/

yarn run rules-omitted
yarn run v1.3.2
$ eslint-index lib/config/all.js  --status omitted
/shopify/eslint-plugin-shopify/node_modules/eslint-index/node_modules/eslint/lib/config/config-file.js:158
        throw e;
        ^

Error: Cannot read config file: /shopify/eslint-plugin-shopify/lib/config/all.js
Error: Cannot find module './rules/prettier'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/shopify/eslint-plugin-shopify/lib/config/all.js:61:5)

• No useless wrappers (<>{stuff}{andThings}</> > <div>{stuff}{andThings}</div>)
• React component should make every member private except React API stuff
• Enzyme no bad features (setState, instance, etc)
• Don't reach across component boundaries (good: ./components, ../components, components, ../OtherComponent. bad: ../OtherComponent/any-path, ./components/SomeComponent)
• Avoid renderFoo methods being explicitly called in render() (use components instead)
• Avoid using most raw DOM manipulation
• Avoid the word correct in test names

Similar to this PR in the old repo: https://github.com/Shopify/javascript/pull/247/files

@lemonmade

WDYT to adding the ignorePureComponents flag to react/prefer-stateless-function? Enabling the flag will still result in an error if you aren't actually using props etc in the component.

Unless you'd rather deter people from using it and recommend they implement scu themselves / use a hoc.

Right now, our eslint config spits out an error if you extend React.PureComponent.

https://github.com/mysticatea/eslint-plugin-eslint-comments

I have a bit of the code written here so let me know if you are working on it.

https://github.com/amilajack/eslint-plugin-compat

https://github.com/facebook/jest/tree/master/packages/eslint-plugin-jest

The following code should be flagged by the linter as requiring var $foo:

var foo = bar || $('baz');

/cc @lemonmade

Originally raised by @lemonmade in the JS repo:

https://github.com/scottnonnenberg/eslint-config-thehelp/blob/3eaed6cda4e14c10df6609885361776c28691a70/scripts/validate.js#L5-L11

The whole repo is also an interesting look at setting up a custom config.

We do not use Jest snapshot tests. It would be nice to enforce this in an eslint rule. Probably makes most sense to contribute it to eslint-plugin-jest. Here's a smiliar-ish rule regarding snapshots

Currently getting a bunch of errors on markup that we should probably whitelist.

/Users/mattseccafien/src/github.com/Shopify/web/app/sections/Products/ProductIndex/components/ImportProductModal/components/PreviewProducts/components/Table/Row.tsx
  10:5  error  replace wrapping tr with fragment shorthand  shopify/jsx-prefer-fragment-wrappers

/Users/mattseccafien/src/github.com/Shopify/web/app/sections/Products/ProductShow/components/VariantsCard/VariantsCard.tsx
  84:7   error  replace wrapping table with fragment shorthand  shopify/jsx-prefer-fragment-wrappers
  86:11  error  replace wrapping tr with fragment shorthand     shopify/jsx-prefer-fragment-wrappers

/Users/mattseccafien/src/github.com/Shopify/web/packages/@shopify/polaris-next/components/DataTable/DataTable.tsx
  102:13  error  replace wrapping thead with fragment shorthand  shopify/jsx-prefer-fragment-wrappers

• Add other HTML elements to valid cases in tests (ie: p, tr, thead, h1, h2)
• Modify check only for divs and spans

Might be good? https://www.npmjs.com/package/eslint-plugin-you-dont-need-lodash-underscore

https://github.com/amilajack/eslint-plugin-compat

I'm interested in using your prefer-class-properties rule, but don't need the other configs/rules. I think because of the way ESLint is structured, it's difficult to use eslint-plugin-shopify and enable just that rule without also having to manually turn 'off' all other things? Any ideas/recommendations?

I guess another option would be to publish just that rule separately as its own eslint plugin. I'd be happy to do that as well but wanted to check in with you guys first.

Thanks for your time!

Prefer class properties to assignment of literals to the class. (prefer-class-properties)

Static class properties allow you to set initial values for class properties, instead of setting these after the declaration of a class.

Rule Details

This rule takes one argument. If it is 'always' (default) then it warns when it finds an assignment to this in a class constructor that could instead be a class property. If set to 'never', it will warn when any class properties are found.

The following assignments are all considered warnings when using the default or explicitly setting the argument to 'always':

class Foo {
}

    Foo.bar = 'bar';
    Foo.baz = 123;
    Foo.qux = {
      foo: 'bar',
      baz: [1, 2, 3];
    };

The following are considered warnings when setting the argument to 'never':

class Foo {
  static baz = 123;
  static qux = something();
}

When Not To Use It

If you do not care how literal class members are assigned, you can safely disable this rule.

Further Reading

• Class properties proposal

https://github.com/mysticatea/eslint-plugin-eslint-comments

Mostly for this one: eslint-comments/no-unused-disable - disallows unused eslint-disable comments

Over the break, I found it very annoying that, in Web (a TS-mostly project), any JS file I was editing had a lesser linting experience: any rules that we turned off because TS handles them were still turned off for JS. I think we can make this better by reworking how our configs are done. We should be able to use glob patterns to have a single plugin:shopify (or similar) config that does what our esnext config does now, and provides overrides for .ts(x)? files. Similarly, we should update our ava/ mocha configs to only apply to test files (this might not be entirely possible if we don't have a common pattern, but if we ever add a Jest-specific config we could do it using the known .test.js|ts|tsx pattern).

cc/ @ismail-syed @GoodForOneFare

"shopify/jsx-no-hardcoded-content": ['error', {
        "allowStrings": false,
        "allowNumbers": false
      }]

does not lint correctly. It throws and error for all hardcoded values in JSX.

Example:
I see the following error:
9:5 error Do not use hardcoded content as the children of the div component shopify/jsx-no-hardcoded-content

And this is my code

function Preview() {
  return (
    <div className={styles.Preview}>
      <div className={styles.Content} />
    </div>
  );
}

Similarly there are many such cases where it shouldn't throw an error

We have GraphQL fixtures in our project and we include these fixtures like that :
import basicFixture from './fixtures/TestAdjacentResourcesQuery/basic.json';

Because TestAdjacentResourcesQuery folder is PascalCase, the rule thinks it is a component but is not. We use PascalCase for that because the GraphQL file related to this fixture look like TestAdjacentResourcesQuery.graphql

Maybe we should exclude fixtures folder.

Originally raised by @lemonmade in the JS repo (here)

https://github.com/eslint/typescript-eslint-parser
https://github.com/nzakas/eslint-plugin-typescript

See https://www.npmjs.com/package/eslint-plugin-jsx-a11y

I've been meaning to use it on polaris-styleguide but it probably would be even better if it was embedded in our default config.

@svinkle is this something you'd be interested in taking on?

Running lint on this file https://github.com/Shopify/online-store-web/pull/893/files#diff-c43e4dc1f934bc937bc5b0a810d6412cR1 leads to react/no-this-in-sfc eslint failures. error: Stateless functional components should not use this. This is obviously incorrect since this module has nothing to do with React.

I like the syntax of the merge() function, but should we use Object.assign() instead?

`npm install eslint-plugin-shopify --save-dev
npm WARN [email protected] requires a peer of typescript@* but none is installed. You must install peer dependencies yourself.
npm WARN [email protected] No description
npm WARN [email protected] No repository field.
npm WARN [email protected] No license field.

• [email protected]
  updated 1 package in 9.185s`

I also see the folder inside node_modules.

However, I keep seeing:

Cannot Find Module eslint-plugin-shopify

.eslintrc is:
{ "extends": [ "plugin:shopify/esnext", "plugin:shopify/jquery", "plugin:shopify/prettier" ] }

Originally raised by @lemonmade in the JS repo:

https://github.com/sindresorhus/eslint-plugin-unicorn
https://github.com/apollostack/eslint-plugin-graphql
https://github.com/jhusain/eslint-plugin-immutable
https://www.npmjs.com/package/eslint-plugin-no-loops

We can prevent the following from being flagged invalid by using the util identified in this discussion, #117 (comment)

import {Subscription} from 'apollo-client/util/Observable';

Consider the following example:

function Table({content, otherContent}) {
 return (
   <table>
     <tbody>
       <tr>{content}</tr>
       <td>{otherContent}</td>
     </tbody>
   </table>
  )
}

The rule currently would consider all of those tags invalid. I think we should remove all the tag checking logic to just look for divs and spans.

@lemonmade what do you think?

Usually better to do a component or just fold into render

https://github.com/BrainMaestro/eslint-plugin-optimize-regex

Looks legit. Try it out on Admin and see if it flags sensible things.

https://github.com/Shopify/web/pull/5020#discussion_r206543532

Maybe just use this https://github.com/blakeembrey/pluralize

Violations just have a prosaic version of the rule name:

Users should not have to visit a rule's homepage to figure out a fix.

I can't tell if the strict-component-boundaries rule is working as intended in this case. These are utils, not components.

Here's the errors in CI for this code.

If this isn't a bug, please close this.
cc/ @cartogram

A common pattern found in test files. The index.ts should be reserved for exposing pieces of the component to the outside application while test files themselves should import from the individual file they are testing.

// bad
import {Button} from '..';

// good
import {Button} from '../Button';

plugin:shopify/typescript-prettier and plugin:shopify/prettier pass Prettier config inline overriding any external config. It can be worked around but it took me a few hours to figure out why:

  prettier/prettier:
    - error
    - parser: typescript

I you have an ES2016 async function that returns a jQuery object, and you assign the await result of calling that function to a variable starting with $, the check complains.

Example:

async function foo() {
  return $(document.body);
}

let $foo = await foo(); // Not allowed

• Enums use pascal case
• Don't use .tsx without React

Seems like a no-brained for the node preset.

https://github.com/nodesecurity/eslint-plugin-security/blob/master/README.md

Might be good? https://github.com/mikemaccana/eslint-plugin-must-use-await

Originally raised by @mikkoh here:

eslint-plugin-shopify has a peer dependency for eslint 3.3.x. The current version is at 3.4.x. It would be nice to have greenkeeper create Pull Requests to get updates for peer deps or locked modules. It's safe to do this... Although the way that this repo is setup as a mono package might cause issues with greenkeeper

Chris responded with this:

I'm open to doing it, but I have a somewhat workable system of just going through every few weeks, bumping the versions on all the ESLint/ plugin dependencies, and adding configuration for all new rules as I do that.

Occasionally components need to import their sibling components

E.g. in src/components/Banner/Banner.ts: importing a Button component import Button from './Button';

Currently this does not flag up an error. Should this be considered a boundary violation to make people change to use import {Button} from '..';

//cc @cartogram