SPFx v1.15+: Consider dialing back ESLint rules

[andrewconnell]

In the latest release of the SharePoint Framework (SPFx), v1.15, Microsoft ditched TSLint in favor of ESLint. While this is a big improvement and a step in the right direction, I think Microsoft went overboard as they added a ton of additional linting rules we didn’t have to deal with before.

Let me be clear: I like ESLint and I think having rules to either enforce coding standards and/or good practices is a good thing. I like that SPFx projects now use ESLint over the long-deprecated TSLint.

What I don’t like is a vendor telling developers how they should be writing my code, especially a vendor imposing subjective coding styles like how we name variables, or that we use the default member accessors rather than spelling it out.

This is an overreaching position by Microsoft, a disappointing one and that I hope they change in a future SPFx release. Microsoft, or any vendor for that matter, shouldn’t include opinionated linting rules in default projects that could impact a developer, or a development team’s, coding style. That’s not their role; that’s up to the developer, dev team, or customer. Otherwise, Microsoft sounds a lot like Steve Jobs telling me I’m holding my phone wrong.

Some of the rules Microsoft is imposing on SPFx v1.15 projects are irrelevant and obsolete. For a group that has a history of not keeping their platform current and using outdated tools (Node, React, webpack, TSLint, etc), we shouldn’t be looking for coding style & guidance from the SharePoint team.

I’m starting this discussion item in an effort to initiate an open conversation with the community with the hope to have a solution that both Microsoft & the community live with.

How default SPFx projects should look

Certain rules they’ve included make a lot of sense andare welcomed. Rules like floating promises, or no-unused-variables are good and promote good coding practices without venturing into much of an argument.

But other rules, like naming-convention, or explicit-member-accessibility, or no-parameter-properties… those are opinionated and up for debate. In those cases, I don’t think a vendor should be imposing those rules on development teams. Rather, that’s the responsibility & prerogative of the developers or companies to set their own standards.

Yes, ESLint provides ways to disable rules (I cover them in a longer article & video), but default projects shouldn’t require teams to make changes before they start being productive with their solution.

ESLint rule change requests on default SPFx projects

I’m going to enumerate some rules that I propose should be removed / disabled from a default SPFx project.

@typescript-eslint/no-explicit-any

The the rule no-explicit-any rule says you can’t use the any type on any of your objects. For instance, what if you don’t know the type? That’s one of the benefits of JavaScript, and thus TypeScript. It supports stuff like this:

export interface IMission {
  vehicles?: any[];
}

To satisfy the rule, create an interface that effectively said that object can have any named property that’s an object:

export interface IAnything {
  [property: string]: object;
}

export interface IMission {
  vehicles?: IAnything[];
}

Did that fix the the ESLint issue message? Sure… the code is harder to read. This rule should be up to the developer.

👉 Please disable @typescript-eslint/no-explicit-any.

@typescript-eslint/consistent-type-assertions

This consistent-type-assertions rule enforces consistent usage of type assertions. For example, you shouldn’t mix assertion styles like these two options:

// angle bracket style
private static _missions: IMission[] = <IMission[]>[];
// as style
private static _missions: IMission[] = [] as IMission[];

This is an opinion… there’s no problem with both lines above. SPFx projects shouldn’t dictate this style.

👉 Please disable @typescript-eslint/consistent-type-assertions.

@typescript-eslint/explicit-member-accessibility

The explicit-member-accessibility rule says you must explicitly specify accessibility modifiers on class methods and properties. For example, the following code will include a message for this rule:

constructor(private someString = 'helloWorld') { }

The problem with the above code is that its missing the public prefix on the constructor. In TypeScript, if you don’t use the accessor, public is assumed. That’s generally known it’s excessive to put it there.

While this one debatable, but seems like an unnecessary subjective opinion that is being imposed on my coding preferences.

👉 Please disable @typescript-eslint/explicit-member-accessibility.

@typescript-eslint/no-parameter-properties

The rule no-parameter-properties “disallows the use of parameter properties in class constructors… Parameter properties can be confusing to those new to TypeScript as they are less explicit than other ways of declaring and initializing class members.”

constructor(private someString = 'helloWorld') { }

TypeScript includes a shorthand syntax that the code above is taking advantage of. What it does is create a private member on the class and sets its value to the value of the parameter passed in. So, the above code can be written as follows to eliminate this ESLint rule message:

private someString: string;

public constructor(someStringArg = 'helloWorld') {
  this.someString = someStringArg;
}

The original code is succinct and easy to comprehend, not to mention it’s using a language feature. The rewritten code takes more time to read and comprehend. Again, another subjective opinion that I don’t think a vendor should be imposing on a developer or a development team who may have their own coding standards.

👉 Please disable @typescript-eslint/no-parameter-properties.

@typescript-eslint/typedef

The other rule the above constructor violates is the typedef rule. This says we must use type annotations anywhere a variable is defined.

In this case, the argument passed in should include the : string type annotation in it’s definition:

constructor(someStringArg: string = 'helloWorld') { }

This isn’t necessary… it’s clear to the compiler & anyone reading the code that the type of someStringArg is a string because the default value it’s set to is a string. Why add extra code you have to comprehend? The goal isn’t to make JavaScript/TypeScript more like C++, the goal is to encourage good practices.

👉 Please remove @typescript-eslint/typedef.

@rushstack/eslint-plugin/no-async-await

Plainly stated, this rule is obsolete. It states that you shouldn’t use the async keyword because it can have performance issues in older browsers. So… this code will raise this issue:

private async _getSiteTitle(): Promise<string> { }

Uh… we’re transpiring TypeScript to ES5 (see tsconfig.json) in your SPFx projects, and async isn’t even in ES5, so the generated JavaScript doesn’t even have that keyword in it. Therefore, it will never touch the browser, so this error is a false positive.

👉 Please remove @rushstack/eslint-plugin/no-async-await.

@jsx-eslint/eslint-plugin-react/jsx-no-bind

The react/jsx-no-bind rule states we shouldn’t ever use arrow functions in our React properties. For example, this code breaks this rule:

<a href="#" onClick={(event) => handleOnRemoveClick(event)}>
  Remove Mission
</a>

But here’s the thing… this is related to a performance issue using the .bind() method. That perf issue was fixed in Chromium v49… that’s from March 2016!

Who disables updates on evergreen browsers? Today most of us are Chromium v103. So not only is this rule obsolete, it’s unnecessary.

Don’t believe me? Then trust what Ryan Florence, the author behind the React Router, said in his blog post: **React, Inline Functions, and Performance.

👉  Please remove @jsx-eslint/eslint-plugin-react/jsx-no-bind.

Point/counterpoint

The above post started from an internal discussion with the SPFx engineering team. A few points from that discussion came up that I want to include here for the benefit of the discussion:

“Believe me or not, rules are good for most devs out there.” & “If we can help people write safe, robust, secure, etc. code by default, we should do that”

Let's take both of these arguments to the logical conclusion: testing is good for all devs & projects out there, so why not go ahead and pack in jest, some sample tests, set rules to force certain code coverage %'s, force me to use Cypress for some UI testing... because tested projects are better than non-tested projects.

If you can help people write safe, robust, secure & testable code by default, you should do that... right?

NO, I don't think so... there's a balance.

At least give developers a way to opt in or opt out of this (you already do this with the minimal template)... esp with the number that are being imposed compared to TSLint.

But nagging build outputs with lining & lack of tests... IMHO, you hurt the adoption & first experience, and confuse people who thing they need to get rid of all these things before publishing their projects, and you impose your opinions on what practices some developers do that are perfectly fine (like using the private keyword in my constructor's arguments as a shorthand.

“We use TypeScript because it makes it safer and less likely to have issues”

TypeScript was created to make large JavaScript projects more maintainable, straight from the creator's original statements announcing it. One of the benefits of JavaScript is its flexibility... TypeScript allows you to put some structure around it, but you can also go overboard with it, and the default rules that are set are doing just that IMHO.

[andrewconnell]

How's this for a solution... Microsoft can use the settings they like... but developers can override the settings with a set of their preferred settings. If they do nothing, they get MSFT's rules as they are in 1.15. But if they create the file in the well-known path, they use their settings.

TL;DR

Proposed solution

In SPFx v1.15.1+, change the .eslintrc.js file to be the following

require('@rushstack/eslint-config/patch/modern-module-resolution');

const fs = require('fs');
const path = require('path');
const myRules = path.join(require('os').homedir(),'.spfx-eslint-rules.js');
const baseEslintConfig = {
  extends: ['@microsoft/eslint-config-spfx/lib/profiles/react'],
  parserOptions: { tsconfigRootDir: __dirname }
};
const esLintConfig = (fs.existsSync(myRules))
  // user-defined rules
  ? { ...baseEslintConfig, rules: require(myRules) }
  // Microsoft defined rules
  : { ...baseEslintConfig };

module.exports = esLintConfig;

As is, this will work in existing SPFx v1.15 projects. But add the following file JSON to the file ~/.spfx-eslint-rules.js:

module.exports = {
  "@microsoft/spfx/no-async-await": "off",
  "react/jsx-no-bind": "off"
}

Now, when I run this in a SPFx v1.15 project where I'm using the async keyword & arrow functions in a property of a React control. The first time, no lining errors. Now delete the JSON file you just created, or change its name. You see listing messages because it used MS's config:

... and if the dev uses ESLint in other places in addition to SPFx projects, if you want to use those, you can always pull them in and add to them, modify them, or whatever. So their ~/.spfx-eslint-rules.js would look like:

const path = require('path');
module.exports = require(path.join(require('os').homedir(),'.eslint-rules.js'));

[amigax]

but where is .spfx-eslintrc.js?

[amigax]

these solutions dont work for me ;-/

[andrewconnell]

@amigax said:

but where is .spfx-eslintrc.js?

It doesn't exist... that was mentioned as part of my pitch... Microsoft didn't adopt my proposal. What I outlined in this thread was just feedback & a proposal.

[amigax]

Ah thanks. I actually just wiped the lint file altogether. Left it.empty and things started working. Dangerous maybe lol.

[andrewconnell]

Not dangerous... linting ONLY focuses on code quality & conventions. Originally linters were necessary before compilers could report where errors were in your code, but we're 30/40/50 years past those days. Still... it's a good tool.

Suggest reading 👉 https://www.voitanos.io/blog/navigate-eslint-sharepoint-framework-projects-guidance/

[AJIXuMuK]

Thank you for the feedback!
Will it work if we disable some of the proposed rules + explicitly list all the rules directly in .eslintrc.js?
In that case you could just easily remove/add whatever rules you want

[andrewconnell]

My solution is a little rough as I'm unclear about the react profile vs. default profile (since I think I saw a thread where that was a known issue in 1.15... I could be wrong...)

I think ideally it would be the MSFT config or the user config and not be merged. Above, I merge two configurations, but to just keep it simple instead of handling a merge just return either the default config or the user's config, maybe something like this would be more appropriate:

require('@rushstack/eslint-config/patch/modern-module-resolution');

const fs = require('fs');
const path = require('path');
const userEslintConfig = path.join(require('os').homedir(),'.spfx-eslintrc.js');
const defaultEslintConfig = {
  extends: ['@microsoft/eslint-config-spfx/lib/profiles/react'],
  parserOptions: { tsconfigRootDir: __dirname }
  rules: { // MSFT's rules }
};
const esLintConfig = (fs.existsSync(userEslintConfig))
  // user-defined config
  ? { ...userEslintConfig }
  // Microsoft defined config
  : { ...defaultEslintConfig };

module.exports = esLintConfig;

Then, the user's config would be the entire config... not just the rules. This would let the user define any other configuration settings they may wish to set... like maybe they want to ignore linting the files that contain their tests/specs.

I get the parserOptions and extends may not need to be set differently for each project. But a solution that hands the config to the user would be the most fool-proof.

[andrewconnell]

Actually, what about a concept more like webpack? If we want to change the webpack configuration from what the build toolchain generates, we can do that using the mergeConfiguration() method. Then we just massage it as we see fit and return it back.

So... same concept here... If a well-known file exists, MS passes it your MS default configuration. The user's job is to implement a method, massage your configuration, and return it back. If I screw it up, that's on me.

So... something like:

./.eslintrc.js

require('@rushstack/eslint-config/patch/modern-module-resolution');
const defaultEslintConfig = {
  extends: ['@microsoft/eslint-config-spfx/lib/profiles/react'],
  parserOptions: { tsconfigRootDir: __dirname }
  /* rules: { MSFT's rules } */
};

const fs = require('fs');
const path = require('path');
const os = require('os');
const userEslintConfig = path.join(os.homedir(),'.spfx-eslintrc.js');
module.exports = (fs.existsSync(userEslintConfig))
  // user-defined config
  ? require(userEslintConfig).mergeEslintConfig(defaultEslintConfig)
  // Microsoft default esLintConfig
  : defaultEslintConfig;

~/.spfx-eslintrc.js

exports.mergeEslintConfig = (esLintConfig) => {
  // change ignore patters
  esLintConfig.ignorePatterns = [
    '**/src/**/*.spec.ts',
    '**/src/**/*.spec.tsx'
  ];
  // change rules
  esLintConfig.rules = {
    "@microsoft/spfx/no-async-await": "off",
    "react/jsx-no-bind": "off"
  };

  return esLintConfig;
};

[AJIXuMuK]

The idea with user-defined config is great for a dev working on separate projects where he wants to have the same config for all of them (and all new).
But for a multi dev tenant rules that are baked in the project should not be overridden by local ones.
So, having this code to merge configs by default in the generator may cause issues in big projects.
We can potentially have some docs around this approach.

But including it in the scaffolded project seems concerning.

[stevebeauge]

If it can help, a simpler way to tweaks rules is to override rules for TS files :

.eslintrc.js :

require('@rushstack/eslint-config/patch/modern-module-resolution');
module.exports = {
    extends: ['@microsoft/eslint-config-spfx/lib/profiles/react'],
    parserOptions: { tsconfigRootDir: __dirname },
    ignorePatterns: ['**/node_modules', '**/*.js', '**/*.json', '**/*.scss'],
    overrides: [
        {
            files: ['*.ts', '*.tsx'], // Your TypeScript files extension

            parserOptions: {
                project: ['./tsconfig.json'], // Specify it only for TypeScript files
            },

            rules: {
                'react-hooks/exhaustive-deps': 'error',
                '@typescript-eslint/no-unused-vars': 'warn',
                'react/prop-types': 'off',
                'react/display-name': 'off',
                'no-useless-escape': 'off',
                '@microsoft/spfx/no-async-await': 'off',
                '@typescript-eslint/typedef': 'off',
                '@typescript-eslint/no-floating-promises': 'warn',
            },
        },
    ],
};

[Ofer-Gal]

I am for @andrewconnell proposal

[salascz]

Hello,
just want to thank @andrewconnell to open the discussion. I agreed with most of his recommendation.

ESLint is a great improvement for SPFx, but it will take me a lot of time to tweak the configuration and get the same results we had for TSLint.

Don't want to describe all rules, here is one rule I do not fully understand:

• naming-convention: Why to force coding style (e.g.: leading underscores) to someone who is starting his journey with SPFx. Used configuration does not seem to be recommended on official coding guidelines for TS or samples for beginners.

  Such a practice seems to be out-of-date from my point of view, but its just my opinion :)

Thanks

[michaelsoriano]

Thank you @andrewconnell - I agree. Default rules should be dialed down. 👍