Work towards stricter linting for app code

[atjn]

If I enable the recommended Eslint checks that ensure variables are scoped and used correctly, I receive 6500 errors (on average about 10 errors per app). That seems problematic to me, especially because these errors are likely performance/power drain issues.

As I see it, you should not be able to merge apps here if they don't pass these checks. The checks are not hard to pass, it is things like deleting variables that are never used, and declaring variables so they aren't accidentally attached to the global scope.

Unfortunately there are so many errors that it is not trivial to go through them all and just fix the issues. Therefore I propose that the global Eslint rules are updated to be more strict, but then each app that doesn't pass the new rules will get its own local Eslint config that disables the new rules only for that app.

This solution would ensure that:

• New apps must pass the stricter rules, so the problem does not continue to grow
• App owners will become aware that their app has an issue, so some will probably go and fix that
• People like you and me can incrementally go through existing apps and fix them on a case-by-case basis

If you agree with this, I would be happy to write a script that automatically adds the necessary local Eslint configs.

As for which rules should be enabled, I think that all rules in the eslint:recommended set should be enforced for new apps.

I am eager to hear your opinion on this! Thanks for taking the time to read my wall of text :)

[atjn]

Related: #450

[nxdefiant]

This will probably mean less people contributing.

I'm not a fan of this because of the following use case. Lets say I want to update an app. Fix a bug, spelling error or add extra functions. But the app basically runs fine (false positive error). That would mean I still would have to change it, which means: Extra work and testing time and possibly risking to break something.

[atjn]

@nxdefiant I understand your concern, and it is definitely necessary to weigh the pros and cons.

I personally think it is a net benefit. The requirements can add a bit more development time, but as I mentioned, the errors are usually easy to solve. For example, delete a variable that you don't use, or put "let" in front of a variable. All Eslint errors have a simple page that explains how to fix the error so most people should be able to follow. It is also worth noting that this repo already enforces a lot of Eslint rules, and as far as I can tell, nobody is complaining about it.

On the positive side, Eslint rules help you write software that is less buggy, so in theory you are more likely to contribute because you don't get stuck on an bug that you can't fix. It also means that the code you write is easier to read for other people, and that could help boost contributions as well.

[bobrippling]

Thanks for the input and PRs - I think this can depend very much on the indivudal lints, for prior discussions, the one in #2810 is perhaps the best for going over the rationale, which can be summarised as enabling developers who don't even have a local dev setup.

@gfwilliams would you say this is still the case?

One option would be to enable it for apps that would generally use a local dev setup, or apps you've created, in a similar manner to the typescript setup.

[atjn]

@bobrippling thank you for the feedback. I am not sure I understand your argument. It seems like you are arguing against adding a linter, but the project already has a linter. My proposals do not add any new linting system, they only add a few extra rules to the 40+ rules that are already enforced for all apps.

When I read through #450, it seems to me like these rules were always meant to be enabled, but nobody got around to doing it.

If you want to develop without a local dev setup, your experience will not change. If you submit code that fails some of the linting rules, GitHub will show the errors in the online code editor, and you can fix it right in the editor without ever opening something local. The experience is almost the same as receiving feedback from a maintainer, except the feedback is instantaneous.

[bobrippling]

Yeah I'm happy either way now, interested in what Gordon thinks with his original argument compared to #450

[bobrippling]

My opinion now is that we have more linting enabled so long as it doesn't break the build, for example what we have with warnings from sanitycheck. I'd still like to hear Gordon's opinion, but it's a nice feature that github can show these warnings, which I imagine wasn't the case when the issue was original discussed

[gfwilliams]

Right now we do have quite a lot of sensible linting, I'm all for adding more when it makes sense, but there are limits, and I definitely don't want to be in a position where we have per-app linting rules and enforce overly strict rules for new apps (because as mentioned it'll put off contributors). I also don't agree that all rules should be enforced as IMO not every lint rule is useful/helpful especially in an embedded environment (and may even be pushing people to write code that is not optimal on Espruino).

We will also likely end up with cases where (like right now with flightdash settings in master https://github.com/espruino/BangleApps/actions/runs/7928167203/job/21645894909) new contributors aren't aware (despite there being warning markers in the code) and don't fix everything, so the linting (in this case sanitycheck) picks up an issue that isn't fixed and gets merged in anyway, then one of the maintainers has to fix it.

I think my question really is: why do you want to enforce these lint rules?

Have you personally experienced bugs in apps in this repo that would have been caught by something that wasn't enabled in the linter?

Because if not, I'm not sure we should be making everyone's life more difficult by adding arbitrary restrictions...

So basically:

• I don't think we should have per-app lint rules
• I don't think we should enable all lint rules by default
• If you find a lint rule that you think makes sense to add because it will actually catch bugs or improve performance, please ask about it and we can maybe add it (but in some cases like with delete it may just not make sense)
• But IMO most importantly we have the sanitycheck file which does some Bangle.js-related checks, and actually catches bugs that we know have caused problems in the past. Adding some more checks into there (or maybe if it's possible by extending lint) based on real issues contributed apps have had would be super helpful.

For example...

• Using require for libraries that are provided by other apps, where a dependency hasn't been put in metadata.json
• Writing storage files where it's not listed in data in metadata.json
• It might be possible to check JS apps against the Typescript decls to ensure that built-in methods are being called with the correct values - I think @bobrippling mentioned this a while back?

[bobrippling]

Yes, the typescript-checks-on-JS would be nice, haven't gone down that rabbit hole enough to have anything on it though

[atjn]

Thank you for the feedback @gfwilliams :)

I think we disagree because we haven't defined exactly what rules we are talking about. I would like to limit the conversation to a single rule that I think is the most important to enable. Then I might be able to create PR's/issues for the other rules in the future.

The no-unused-vars rule ensures that the code does not define variables that are never used. All the apps currently declare 1120 unused variables. I think this rule should be enforced because:

• Removing these variables would make the programs run faster, use less memory, use less battery.
  • Admittedly, many of these variables are undefined and thus will only make a small difference, but some of them either use a lot of memory, call functions to derive their value, or import entire modules.
• Removing these variables catches bugs. If you define a function or variable, you probably expect it to be used, and it not being used is often associated with a bug/oversight in your code. Being made aware of these issues will help you catch those bugs.
• Removing these variables are a big improvement to readability. Unused declarations clutter your understanding of what the code exactly does.
• It is very easy to adhere to this rule. You can just remove the variable declaration, and you are guaranteed to not have altered the program. If you want to keep it for whatever reason, you can always comment it out.

If you don't want to have per-app linting rules, then maybe we can enable it as a warning?

I'm not sure we should be making everyone's life more difficult by adding arbitrary restrictions... [...] I don't think we should enable all lint rules by default

Just to be clear, I am only asking to enable the eslint recommended rules. I would not describe them as arbitrary, as each of them exist because developers have seen a lot of issues arise from the pattern that the rule is disallowing.

I think my question really is: why do you want to enforce these lint rules? Have you personally experienced bugs in apps in this repo that would have been caught by something that wasn't enabled in the linter?

I have not personally experienced bugs, but I have been looking through the code of other apps to figure out how I should write my own app. In the process, I found several cases of apps that were written with dead code / unintuitive code that misled me and made me misunderstand what the code was doing. I looked into this and found issue #450 which suggested to me that you wanted to have these rules enabled. Given that it would help me read the code and would improve code quality, which also helps me, I started looking at what I could do to enable these rules :)

[gfwilliams]

A warning for no-unused-vars sounds like a good idea.

Looking at the result of no-unused-vars I'm not particularly happy with the warning over unused function arguments. For instance we might do Bangle.on("touch", e => ...) - it's kind of handy knowing that there's a potential argument e there if you want to check the coordinates at some point in the future and I don't think we should force removal of those named args.

So "no-unused-vars": ["warn", { "args": "none" } ], brings us down to 600 warnings which is slightly more reasonable.

I do see some warnings caused by code that saves the result of calling setInterval - and that doesn't feel like such a terrible thing to do even if the var isn't actually used... But yes, on the whole I think the majority of these warnings are pointing to things that would tidy up the code and maybe even save a bit of memory.

You can just remove the variable declaration, and you are guaranteed to not have altered the program

I guess we will see! Part of me thinks that if we change 100s of apps in one go, there's a high likelihood that something will break. I think at least we should bump the version numbers of changed apps so we can keep track and be sure that updates are actually sent to users.

Anyone else have any views on this? @bobrippling @halemmerich @thyttan ? I guess this may end up causing extra work for you if there do happen to be side-effects

[atjn]

Looking at the result of no-unused-vars I'm not particularly happy with the warning over unused function arguments. For instance we might do Bangle.on("touch", e => ...) - it's kind of handy knowing that there's a potential argument e there if you want to check the coordinates at some point in the future and I don't think we should force removal of those named args.

I would like to understand your thought process here. In #3186 you say that we need a method to completely delete variables because even a variable that is undefined still uses too much memory on some devices. But in this case you think it is fine to initialize 500 variables that are never used. If it is true that even a single undefined variable is problematic, then this seems like a major issue that we should be working hard to solve. Am I misunderstanding something here?

I guess we will see! Part of me thinks that if we change 100s of apps in one go, there's a high likelihood that something will break. I think at least we should bump the version numbers of changed apps so we can keep track and be sure that updates are actually sent to users.

Yeah that is the problem that I am trying to address by having per-app linting rules. You wouldn't need to have this big rush where we risk breaking stuff. But I am happy either way :)

[gfwilliams]

But in this case you think it is fine to initialize 500 variables that are never used

If you install all 600 apps and run them at once, which is impossible?

In reality you're using maybe 1 or 2 variables extra per app (when those functions are defined, which isn't always), which I think is worthwhile to keep readability (something which you have been pushing for).

In #3186 I'm suggesting that we keep deleting things like settings variables, which are potentially 100 or so vars per app. There's a big difference - and even if you set them to undefined they are still sitting there in the global namespace - whereas an extra function argument is almost undetectable.

[bobrippling]

Anyone else have any views on this? @bobrippling @halemmerich @thyttan ?

Yeah I'm happy for non-functionality lints to go in, such as unused variables. I prefer function arguments to be an exception to this for similar reasons, i.e. documentation and clarity of code.

[thyttan]

Anyone else have any views

My gut feeling is I'm sympathetic to it. I also know I'm not the most active in merging PRs at the moment, so that would be a point against as you say @gfwilliams . But I lean towards going forward with it - especially since @bobrippling would like it too.

[atjn]

Thank you all for working to find a consensus here. I have opened #3213 which should apply the new rule :)

[gfwilliams]

Ok, so #3212 looks perfect - nice and clean, no extra warnings.

But as mentioned on #3213 and #3214 we can't just add eslint checks that create thousands of warnings because it's just swamping the CI checks with errors unrelated to the PRs. We might as well just completely remove eslint altogether.

I've had a quick look and I'm not sure there are any great options...

• I'm not happy with the idea of adding thousands of // eslint-disable-line comments to the code, as if pretokenisation isn't turned on by the user those will get uploaded to the watch.
• I know you suggested individual eslint rules, but apart from adding another 600 files to the repo, that itself is far from ideal. It means that the majority of existing apps won't have these new eslint checks applied to them, even when they get modified in the future (which kind of negates the point). The extra file in every app is also confusing to new users and I imagine folks will start copying the file into new apps to let them ignore the new rules too.

On https://github.com/eslint/eslint/issues/13439 it's mentioned that we could:

Iterate through the results, checking for line numbers of reported errors.

So potentially you could make a tool that ran eslint and then looked in a single file of grandfathered errors, and ignored ones that were in that list. Actually even changing the command to eslint | grep -v -x -f existing-errors.txt would do it.

But realistically I don't think anyone is going to fix all those errors if you don't do it, so we're then going to be stuck with this huge file full of ignored errors forever.

So really, the best thing for everyone would be if you could actually fix all the warnings that appear when you turn on a particular check. As you noted in the PRs, you might actually end up fixing existing apps in some cases which would be a huge benefit.

[atjn]

Like you say, there is no obvious great solution, but I still think we can work with some of these.

I like the idea of adding // eslint-disable-line. The app loader has pretokenisation enabled by default, so I think that solves the problem for most users (?). For the people that don't use pretokenisation, they probably don't use most of the apps that this would affect, so the problem is greatly minimized just by that. In the PR, I could fix the warnings for the most used/liked apps to further minimize this impact. And even if you are an unlucky user that doesn't use pretokenisation and has a lot of apps with a lot of warnings, the apps would only increase in size by something like 1% I would guess? This is only a temporary file increase, as you would expect an ongoing commitment to finding and fixing the bugs in the future.

Is it really that bad?

[gfwilliams]

I think my feeling is, most of these warnings are pretty minor fixes - if we're going to change the app's files it should be to actually fix the problem rather than duct-tape over it?

What's your objection to the file of ignored warnings? It also has the benefit that it's a single file that shows exactly which warnings have been ignored, so at some point others might see it and take it upon themselves to fix some of them - maybe.

[atjn]

I think my feeling is, most of these warnings are pretty minor fixes - if we're going to change the app's files it should be to actually fix the problem rather than duct-tape over it?

I understand your feeling, but the practical problem is that these are not minor fixes when you need to properly understand and test every single app. So if duct-taping each app means that apps in the future will be less buggy and more performant, then I see no problem.

What's your objection to the file of ignored warnings?

I think there are pros and cons to either of those solutions. In general I think a file of ignored warnings is a good solution and I would be happy to implement it.

1) It is problematic that the ignored warnings are not shown in the code, so developers will not be aware that there is a problem that needs fixing. This could lead to the situation that issues are never fixed. You might solve this by creating an issue with all the apps that need fixing, and making it a community effort to fix them. You could also tag the authors of each app to see if they want to help.

2) To correctly ignore warnings, I think you would need to include the line number of each warning. This would mean that if I just need to edit a small chunk of code and I add a single line near the top of a file, then all warnings from the entire file will be enabled. Maybe this is actually a feature; if you want to edit the file, you must fix existing issues first. You can always solve this on a case-by-case basis by updating the known warnings file with the new line numbers.

[gfwilliams]

if I just need to edit a small chunk of code and I add a single line near the top of a file, then all warnings from the entire file will be enabled

Yes, that had occurred to me and it felt like a good thing. In fact if we were able to md5sum all files and then only lint changed files, that could be an option too - so any change to a file would require all warnings in that file to be fixed (which usually would only take a few minutes for one app - it's just the sheer volume of apps that is daunting).

But either way I think that any warnings picked up by the linter that are easy to fix without repercussions should be fixed when these lint changes go in. Some may not be clear and maybe those need an exception added, but they should be the minority.

@bobrippling any thoughts on options for this?

[atjn]

But either way I think that any warnings picked up by the linter that are easy to fix without repercussions should be fixed when these lint changes go in. Some may not be clear and maybe those need an exception added, but they should be the minority.

I can fix all warnings that I think are trivial and could never cause a bug, but I would not commit to testing most of the apps. I think there are more non-trivial issues than you expect.

[gfwilliams]

Thanks, yes - that sounds fair. Testing all of them is a pretty much impossible job :)

[bobrippling]

Yeah I like the sound of that - a list of grandfathered files which we could store with their md5, then if there's a change it gets booted from the grandfather list and the committer should fix the lints. This kind of approach has worked well on work projects I've been on.

I can't remember if there was a way to get eslint to work with a grandfathered list, otherwise there would be some infrastructure needed around the md5ing. @atjn do you think an initial draft is something you'd be able to put together, if @gfwilliams you agree?

[atjn]

Yes I'll take a look at it. If file hashes / app hashes are not possible, then I will try the known warnings file.

[gfwilliams]

Yes, that all sounds good to me.