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.
Today I published a video on the Voitanos YouTube channel that demonstrates how to cope with some common ESLint rules you’ll likely encounter when migrating your existing SPFx projects to SPFx v1.15.
In that video, I show you how to understand what a linting issue is telling you, how to change your code, or how to ignore the rule in your projects.
But that’s not all…
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 me how I should be writing my code, especially a vendor imposing subjective coding styles like how I name my variables, or that I use the default member accessors rather than spelling it out.
As someone who uses TypeScript in a lot more than just SPFx projects, I now need to configure every SPFx project I create to be in line with my other TypeScript projects, or I have to accept the fact the SharePoint team’s opinions on how I should write my code should be different for SPFx projects.
I find this to be 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 projects are irrelevant and obsolete (I’ll call some of those out later in the post). 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.
How default SPFx projects should be configured
Certain rules they’ve included make a lot of sense and I welcome them. 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.
Disabling ESLint rules
Before I cover some ESLint rules you’ll encounter in your SPFx projects, let’s cover how you can disable these rules.
ESLint gives us a few options to control if rules should be ignored when we lint our code.
Disable a rule across an entire project
The ESLint configuration file supports disabling or configuring a rule for an entire project. Within the ./.eslintrc.js file, add a rules object with each rule you want to configure or disable.
For example, the following will disable two of the rules I take exception to in the default SPFx v1.15 project & cover later in this post:
require('@rushstack/eslint-config/patch/modern-module-resolution');
module.exports = {
extends: ['@microsoft/eslint-config-spfx/lib/profiles/react'],
parserOptions: { tsconfigRootDir: __dirname },
rules: {
"@microsoft/spfx/no-async-await": "off",
"react/jsx-no-bind": "off"
}
};
Disable ESLint for an entire file
To disable ESLint on a specific file, while you can use the .eslintignore file (similar to a .gitignore file), you can also disable a specific rule within a file.
Simply add the following to the first line in the file:
/* eslint-disable */
Disable a rule for a specific line
Another option you have is to disable a rule on a specific line. To do that, add the following code that includes the rule on the line (in this case, I’m disabling the no-explicit-any rule on my vehicles property:
vehicles?: any[]; // eslint-disable-line @typescript-eslint/no-explicit-any
Disable a rule for a block of code
You can also disable a rule for a block of code. Do this by first disabling the rule, then enabling it at the end of the block:
/* eslint-disable @typescript-eslint/no-explicit-any */
vehicles?: any[];
crew?: any[];
/* eslint-enable @typescript-eslint/no-explicit-any */
Avoid linting in the SPFx build process
The other option is simply to disable linting across your entire project. Do this by adding the following line to your gulpfile.js file before the existing line that initializes the gulp tasks:
// add this line:
build.lintCmd.enabled = false;
// existing code...
build.initialize(require('gulp'));
Common ESLint rule issues in SPFx projects
In the video associated with this post, I cover some common issues you’ll see with SPFx projects. I took an sample project from my Mastering the SharePoint Framework course download and ran it through the default rules for an SPFx v1.15 ESLint configuration. For context, this is a React Hooks based SPFx web part.
The default rules found 18 different issues with this project:
Common ESLint rule findings with an SPFx v1.15 project
Let’s look at a few of these common findings that I think you’ll see in your projects:
@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. In my case, I had an interface like this:
export interface IMission {
id: string;
name: string;
launch_date?: string;
vehicles?: any[];
crew?: any[];
}
Those last two property definitions needed to get fixed according to this rule. While I know the type for the crew, I don’t know what the vehicles are… so the any type is fair to use.
But to satisfy the rule, I just created an interface that effectively said that object can have any named property that’s an object:
export interface ICrew {
role: string;
astronaut_id: string;
}
export interface IAnything {
[property: string]: object;
}
export interface IMission {
id: string;
name: string;
launch_date?: string;
vehicles?: IAnything[];
crew?: ICrew[];
}
Did that fix the the ESLint issue message? Sure… but in my opinion, the code is harder to read so to me, this rule should be up to the developer.
@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[];
Usually I prefer to prefix the object using the angle bracket style because I like to see it before the object. In the above case, the array is huge so having the type 400+ lines after the declaration wasn’t ideal… but… it’s an easy fix.
@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') { }
I’m not a fan of this rule, at least how SPFx project have it set. The problem with the above code is that I’m missing the public prefix on the constructor. In TypeScript, if you don’t use the accessor, public is assumed. That’s generally known so I think it’s excessive to put it there.
I’ll concede this is debatable, but seems like an unnecessary subjective opinion that is being imposed on my coding preferences.
In fact, the above class constructor has two more ESLint issues… let’s look at these…
@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.”
TypeScript includes a shorthand syntax that my 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;
}
My complaint is 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.
@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.
@typescript-eslint/no-floating-promises
This is a good rule… the no-floating-promises ESLint rule. It says when using promises, you should always include code to handle exceptions. For instance, this code will violate this rule:
this._getSiteTitle()
.then((siteTitle: string) => {
this._currentSiteTitle = siteTitle;
});
To address this, I should include a catch statement. But… all you need to do is just that… the catch doesn’t have to do anything:
this._getSiteTitle()
.then((siteTitle: string) => {
this._currentSiteTitle = siteTitle;
})
.catch(() => { });
This is where you need to use that gray matter on your shoulders… should you really just be focused on eliminating these ESLint messages, or should you follow the spirit of what they’re telling you and add a handler?
Sure… you can do the above code and get rid of the rule, but I’d argue you’re missing the plot and have had a negative impact on your code quality. You should really handle the exception, not like how I’ve shown in this code sample.
@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.
I called this out and it seems they may have already fixed it for a future SPFx release, but still, it never should have been there in the first place.
@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!
I don’t know about you, but I don’t know anyone 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.
Conclusion
What do you think? I’ve created a discussion item on their issue, sharepoint/sp-dev-docs/discussions#8290, list to debate this.
Please take a moment and let Microsoft know what you think, either by simply reacting to the post or leave a comment in the discussion. Now’s the time because we might be able to get some things change before the next framework release.