wagtail
☂️ Clean up eslint usage - remove 'legacyCode' ignoring
☂️ This is an epic / umbrella issue We expect this to be done in multiple PRs as it's better to change a few pages at a time, please reference this issue in any new PRs
Description
- As part of the ongoing improvements to the Wagtail frontend codebase the plan is to ensure that new code and existing code can adopt the baseline eslint rules.
- The goal is to incrementally clean up eslint rules, with mostly adopting the rules provided by https://github.com/wagtail/eslint-config-wagtail and adjusting that repo if needed.
- This is so that newer code can be written without ignoring too much and to get to the bottom of which rules are not really 'legacy' rules.
Approach
Part 1 - Remove unnecessary rules or rules that are formatting fixes only
- https://github.com/wagtail/wagtail/pull/8734
- https://github.com/wagtail/wagtail/pull/8736
- https://github.com/wagtail/wagtail/pull/8737
Part 2 - Move some rules to global ignore as they really do not suit existing code approaches
- https://github.com/wagtail/wagtail/pull/8738
- Note: May want to add issues/PRs to adopt some of the global rules in eslint-wagtail
- May be others
Part 3 - Assess small fixes when removing linting rules
- There may be some rules that are quick wins and only require one or two changes/fixes to files
Part 4 - Try to scope overrides rules: legacyCode, to specific files
- There may be rules ignored via
legacyCodethat are harder to fix by only really touch one or two files, try to ignore these explicitly instead of whole folders like entrypoints/utils
Links
- https://github.com/wagtail/wagtail/discussions/7689
Note to self. Clean this description up and add checkboxes for left over items.
@Lovelyfin00 maybe you could give a go at contributing towards this PR.
There are a bunch of legacy ignore rules we are hoping to either ignore formally or remove the ignoring and fix the issue.
One candidate for fixes is 'no-prototype-builtins': 'off',.
https://github.com/wagtail/wagtail/blob/44575387c75c31a6b2113330aba8b55eb51bcbc7/.eslintrc.js#L11
See how you go removing that ignore rule, and then fixing the issues that arise when running linting. This will require some JavaScript knowledge.
Once fixes are applied, you may find that we can remove one or more of the files listed in the legacy override.
https://github.com/wagtail/wagtail/blob/main/.eslintrc.js#L71
@activus-d as per above comment. Maybe you would be interested in tackling the removal of the ignoring of a different rule.
'no-plusplus': 'off',
https://github.com/wagtail/wagtail/blob/44575387c75c31a6b2113330aba8b55eb51bcbc7/.eslintrc.js#L10
Note that we have a range until that may help for most cases where we are using ++
@Lovelyfin00 sorry but I don't think there will be much more to do, based on my investigation I think the other rules will simply be ignored globally instead of kept as rules but code fixed up.
@Lovelyfin00 my apologies - yes. I meant to tag you.
@nutjob4life - please ignore this sorry.
@nutjob4life - please ignore this sorry.
I was just studying up on eslint and was getting ready to dive in! 😝
Oh well, another time! 😁
@activus-d & @Lovelyfin00 - how are you going with these items? Did you need a hand - feel free to ask questions in Slack.
@lb- I’m yet to get started with it because I’m trying to complete tasks assigned to me on for the documentation project. I will definitely get started with it once I’m done.
Yeah Me too I'll get started when I'm doing with the current issue I'm working on
No problems - try not to take on too much and yes focus on your existing items for now.
has-prototype-builtins has been cleaned up via https://github.com/wagtail/wagtail/pull/9634
Hi @lb-
I want to pick up and start working on 'no-plusplus': 'off'
Is that the only rule that is left to be removed on this issue?
@Lovelyfin00
There are a few PRs already up but waiting on other core team to review, and after that I think a few small rules to turn off globally but getting the no-plusplus done will be pretty awesome.
Please note that when we have a for loop, usually it is to solve one of two problems;
- a. doing something for each item in an array - in that case we should try to use
array.forEachhttps://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach - b. doing something for a discrete number of times - in that case we should use the range util (which generates an array with n number o items) AND the forEach from that array.
I know it might be tempting to just change the i++ to i += i but the for loops make the code harder to read and were written in a time where we had to support IE 11.
Be sure to understand how the range util works. range(0, 3) will give you [0, 1, 2] NOT [0, 1, 2, 3]. range(1,5) will give you [ 1, 2, 3, 4 ].
Creates an array of numbers progressing from start up to, but not including, end.
Other open PRs for reference.
- https://github.com/wagtail/wagtail/pull/9482
- https://github.com/wagtail/wagtail/pull/9483
- https://github.com/wagtail/wagtail/pull/8746
@Lovelyfin00 there is one small chunk left, it would be great if you could pick this up if you get the chance this week.
Step 1 - remove legacy rules
This whole block can be removed now.
// Rules which have been enforced in configuration upgrades and flag issues in existing code.
// We need to consider whether to disable those rules permanently, or fix the issues.
const legacyCode = {
'default-param-last': 'off',
'no-continue': 'off',
'no-else-return': 'off',
'no-restricted-syntax': 'off',
};
Step 2 - Remove legacy overrides
This block can also be removed
// Legacy Code - remove from `files` when adopting desired rules in new code progressively
{
files: [
'client/src/entrypoints/admin/core.js',
'client/src/entrypoints/admin/page-editor.js',
'client/src/entrypoints/admin/telepath/widgets.js',
'client/src/entrypoints/contrib/typed_table_block/typed_table_block.js',
'client/src/utils/version.js',
],
rules: legacyCode,
},
Step 3 - Add back two ignore rules for components
Add back no-continue and no-restricted-syntax to the components ignoring - I did a bit of digging and the changes needed here to satisfy these rules would take a long time and will not give us a big benefit.
// Rules that we are ignoring currently due to legacy code in React components only
{
files: ['client/src/components/**'],
rules: {
'no-continue': 'off',
'no-restricted-syntax': 'off',
Step 4 - run fix & format
Now run
npm run lint:js -- --fix
This will run the eslint fixer to fix up a few automatic things (no-else-return items)
Then run
npm run format to clean up any odd formatting from Eslint
Step 5 - Ignore a few inline items
These two files will flag a linting warning default-param-last - but to refactor this code to meet the eslint rules will require a rethink of a key part of the redux structure in this code and is not worth it - just add an inline ignore.
- wagtail/client/src/components/PageExplorer/reducers/nodes.ts
- client/src/components/PageExplorer/reducers/explorer.ts
Try to do it inline like this for them - it reads a bit better
/**
* Contains all of the page nodes in one object.
*/
export default function nodes(
state = defaultState /* eslint-disable-line default-param-last */,
action: Action,
) {
Step 6 - The actual clean up bit :)
Once you have done this, running npm run lint should expose only a few small linting items to fix up.
/client/src/entrypoints/admin/core.js
155:9 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
167:9 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
/client/src/entrypoints/admin/page-editor.js
136:5 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
/client/src/entrypoints/admin/telepath/widgets.js
298:5 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
/client/src/entrypoints/contrib/typed_table_block/typed_table_block.js
460:7 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
461:9 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
513:5 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
514:7 error iterators/generators require regenerator-runtime, which is too heavyweight for this guide to allow them. Separately, loops should be avoided in favor of array iterations no-restricted-syntax
✖ 8 problems (8 errors, 0 warnings)
All of these problems are with the usage of for in... loops, give a go at changing them to array methods (such as myArray.forEach or myArray.map or myArray.some / find).
For example, here is how we could solve the issue in page-editor.js
original
const updateWarnings = () => {
for (const warning of warnings) {
const visible = typeVisibility[warning.dataset.unsavedType];
warning.hidden = !visible;
}
};
proposed
const updateWarnings = () => {
warnings.each((warning) => {
const visible = typeVisibility[warning.dataset.unsavedType];
warning.hidden = !visible; /* eslint-disable-line no-param-reassign */
});
};
I realise this is a bit odd as we are still disabling a rule but there may be a better way with jquery to set a hidden attribute.
Recap
It's 100% ok if we have to ignore a few discrete files for a specific rule here and there. The reason we want to finish this is so that we do not have to much 'blanket' ignoring of entire files or areas of code if it can be avoided.
So even if you end up fixing 1 or 2 of the files from step 6 above - that's all good.