pnp
Enhancement: Pre-populate .eslintrc.js when upgrading SPFx projects to v1.15
With SPFX v1.15 tslint has been replaced with eslint and by default a bunch of more rules are added. When building an upgraded project you are likely to get a whole bunch of eslint warnings which to the average developer can be overwhelming unless you're not bothered by warnings.
eslint is not about code correctness, but is there to help you write more robust code - with a pre-opinionated rule set. I have two proposals.
One
Instead of a blank eslintrc file, add the rules likely to cause warnings, but have them enables as the default 1.15 has. But having them in place makes it easier to turn them off. Something like:
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": 1,
"@typescript-eslint/naming-convention": 1,
"@typescript-eslint/typedef": 1,
"@typescript-eslint/explicit-function-return-type": 1,
"react/jsx-no-bind": 1,
"@typescript-eslint/no-explicit-any": 1,
"@typescript-eslint/no-parameter-properties": [1, {
allows: ["public"]
}],
"max-lines": 1,
"@rushstack/security/no-unsafe-regexp": 1,
"dot-notation": 1,
"prefer-const": 1,
"eqeqeq": 1,
"react/self-closing-comp":1,
}
};
could be more as well.
Two
Have a switch to add the same rules but in a disabled state, or output a message in the output report about taking a peek at the eslintrc file and make up your mind.
Awesome suggestion! @pnp/cli-for-microsoft-365-maintainers we'd appreciate your feedback
Sounds good, I vote for an extra section for optional changes, with the benefits described along with them. This could be used with upgrades to other versions as well. (if we have optional advice like this)
Sounds good, I vote for an extra section for optional changes, with the benefits described along with them. This could be used with upgrades to other versions as well. (if we have optional advice like this)
We already have the notion of optional rules, so this could be one of them. The only challenge is how (if at all) it would show up in the summary script, which is how most folks go about upgrading their project. Alternatively, we could consider introducing a switch, like --extraSuggestions, to include these extra recommendations, but we'd need to see how discoverable it would be.
I'd say not use a switch, but add it anyhow. It would fantastic if the suggestions where appended anyway below their own Suggestions header or something.
Change from using 0/1 => "off"/"on" to be more clear and consistent with existing rules.
Would prefer to have a single configuration, such as one in the default profile on my environment, that the project inherits from, so one doesn't have to configure each project, or uses it as the default.
For instance, why set tabs vs. spaces or font size in a project when it's can be as a setting in the editor.
Additional rules:
- no-floating-promises
- react/jsx-no-bind
- no-explicit-any
- consistent-type-assertions
- explicit-member-accessibility
- naming-convention
Excellent suggestion, I'll love to see it available when upgrading a Project
I also love the idea 😍
Not having the option and just adding it always seems ok 👍
TBH I would add a option for it which will maybe allow us not to only add or not to add empty eslintrc file but maybe also allow the user define how much validation she or he wants to add 🤔. Of course now we will need to discuss what rule should be added to which option 😋.
So I was thinking like adding something like --eslintRules [empty, basic, advance, tooRestrictiveToWriteCode] or something similar. Each of those could pre populate the file with different set of rules (more/less).
Let's keep in mind, that the context is the spfx project upgrade command that runs across all versions of SPFx, so we should be mindful of introducing new options that apply only to specific versions.
If we're to hide the functionality behind an option, then I'd suggest something more generic, like --withRecommendations or --recommendations and which we could use for things other than just eslint.
There's a discussion on sp-dev-docs about this change in SPFx. I'd suggest that we wait until the conclusion is reached so that we better understand what developers would prefer. If you have feedback on the rules/default SPFx project configuration, please chime in the discussion on sp-dev-docs.
@Adam-it said:
TBH I would add a option for it which will maybe allow us not to only add or not to add empty eslintrc file but maybe also allow the user define how much validation she or he wants to add 🤔. Of course now we will need to discuss what rule should be added to which option 😋.
@waldekmastykarz said:
If we're to hide the functionality behind an option, then I'd suggest something more generic, like --withRecommendations or --recommendations and which we could use for things other than just eslint.
Not a fan of this... IMHO, the project upgrade command should be for upgrading a project from one version of SPFx to another version. Going down the path of adding features that modify projects and yield something you woudln't normally get from a brand new project created with the generator is changing this command to be more about project customization than project upgrade.
In general, not a fan of having this tool make changes to .eslintrc.js to silence coding issues the ESLint config is now exposing in SPFx v1.15 projects.
IMHO, this (new ESLint rule messages) should be addressed in the generator itself, hence the discussion item I created... or get my heavily opinionated version from my blog post / ]video 🤷♂️.
Play this out: if the SPFx engineering team makes adjustments to SPFx v1.15.1 that mitigate the default projects to be more in line & yield only critical changes, do you really want to change them here?
IMHO, the only thing this tool should do is not make changes to the project, rather provide guidance if you upgrade a pre-SPFx v1.15 project => a v1.15+ version. Because I don't see much how this differs from if your app used an older & now removed API from the version you're working with. In that case, you have to fix your code.
When it comes down to it, ESLint rule messages are warnings, not errors.
Is this discussion still relevant now that SPFx 1.15.2 has been released some weeks ago?
I don't think so, because the default set has changed and developers can still decide which rules they want to use. Closing for now.