Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ESLint check #2438

Merged
merged 14 commits into from
Jul 3, 2024
Merged

Add ESLint check #2438

merged 14 commits into from
Jul 3, 2024

Conversation

dodieboy
Copy link
Member

@dodieboy dodieboy commented Jul 3, 2024

Added ESLint into the test as mention on #2332 with some config like indent "tab", ignore empty catch and more

@dodieboy
Copy link
Member Author

dodieboy commented Jul 3, 2024

Currently have continue-on-error on and JSLint also not remove, should it be remove?
@raszpl any config you feel need to be added?

There is still allow lint that I did not fix.

@dodieboy
Copy link
Member Author

dodieboy commented Jul 3, 2024

Currently still have 75 errors

@ImprovedTube ImprovedTube merged commit 00d51ad into code-charity:master Jul 3, 2024
1 check passed
@ImprovedTube
Copy link
Member

hi @dodieboy

@raszpl
Copy link
Contributor

raszpl commented Jul 3, 2024

Currently have continue-on-error on and JSLint also not remove, should it be remove?

JSLint is not doing anything at the moment "Stopping. (4% scanned)."
JSLint needs manual help #2415 to be able to tell whats going on

js&css/extension/init.js
 #1 'extension' was used before it was defined.

it doesnt understand (not that I would expect it to) manifest.json so doesnt know

				"js&css/extension/core.js",
				"js&css/extension/functions.js",
				"js&css/extension/www.youtube.com/night-mode/night-mode.js",
				"js&css/extension/www.youtube.com/general/general.js",
				"js&css/extension/www.youtube.com/appearance/sidebar/sidebar.js",
				"js&css/extension/www.youtube.com/appearance/comments/comments.js",
				"js&css/extension/init.js"

init.js is the last file injected and extension was defined all the way back in core.js

var extension = {

but the worst thing about jslint is how opinionated Crockford is leading to some hardcoded nonsense like prohibited Tabs or code indentation. Imo no point keeping JSLint around with ESLint in place.

@raszpl any config you feel need to be added?

at first glance ESLint seem to have a grasp of file order? did you instrument this manually by giving it some sort of a map? for example

/home/runner/work/youtube/youtube/js&css/web-accessible/www.youtube.com/appearance.js
Error:   92:12  error  'i' is already defined  no-redeclare
Error:   92:19  error  'l' is already defined  no-redeclare

it doesnt mindlessly complain about "ImprovedTube not defined". Did you configure it with no-undef? Hmm it doesnt complain about undefined yt.config_.EXPERIMENT_FLAGS.kevlar_watch_grid so definitely no-undef.

Do you know of a way of instrumenting ESLint about execution contexts and order of files? Even if it had explicit support for linking web extensions by parsing manifest.json that would only solve /extension/ part of the extension. /web-accessible/ are manually injected in particular order

extension.inject([
'/js&css/web-accessible/core.js',
'/js&css/web-accessible/functions.js',
'/js&css/web-accessible/www.youtube.com/appearance.js',
'/js&css/web-accessible/www.youtube.com/themes.js',
'/js&css/web-accessible/www.youtube.com/player.js',
'/js&css/web-accessible/www.youtube.com/playlist.js',
'/js&css/web-accessible/www.youtube.com/channel.js',
'/js&css/web-accessible/www.youtube.com/shortcuts.js',
'/js&css/web-accessible/www.youtube.com/blocklist.js',
'/js&css/web-accessible/www.youtube.com/settings.js',
'/js&css/web-accessible/init.js'

and afaik there is no way for linter to know that.
Something like a test_extension.js file containing

import "js&css/extension/core.js";
import "js&css/extension/functions.js";
import "js&css/extension/www.youtube.com/night-mode/night-mode.js";
import "js&css/extension/www.youtube.com/general/general.js";
import "js&css/extension/www.youtube.com/appearance/sidebar/sidebar.js";
import "js&css/extension/www.youtube.com/appearance/comments/comments.js";
import "js&css/extension/init.js";

and test_webaccessible.js

import '/js&css/web-accessible/core.js';
import '/js&css/web-accessible/functions.js';
import '/js&css/web-accessible/www.youtube.com/appearance.js';
import '/js&css/web-accessible/www.youtube.com/themes.js';
import '/js&css/web-accessible/www.youtube.com/player.js';
import '/js&css/web-accessible/www.youtube.com/playlist.js';
import '/js&css/web-accessible/www.youtube.com/channel.js';
import '/js&css/web-accessible/www.youtube.com/shortcuts.js';
import '/js&css/web-accessible/www.youtube.com/blocklist.js';
import '/js&css/web-accessible/www.youtube.com/settings.js';
import '/js&css/web-accessible/init.js';

with ESLint running only over those two files?

Problem: There are more no-redeclare errors in appearance.js later in the file, like

container = document.querySelector("#related ytd-compact-video-renderer.style-scope")?.parentElement;

that it fails to flag, I dont know why yet.

Also doesnt complain about stray spaces [no-multi-spaces], this is super minor :)

Personally I really dont like the Java switch indent rules, much prefer saner php ones as they actually make Switch/case visible from far without reading the code. Imo Switch statement https://www.crockford.com/code.html "This avoids over-indentation." is a huge crock of bullshit :) JSLint is very badly opinionated in this way :( beautifier/js-beautify#15 also agrees with me and indents 'case' properly, can check it out at https://beautifier.io. You edited a ton of files crushing Switch statements according to Crockford fantasy :( :)

There is still allow lint that I did not fix.
Currently still have 75 errors

Everything doesnt have to pass from the get go, we can have few months of linter not passing while we systematically fix errors.
Imo fixing errors should have been next patch , but what is done is done :) Thank you for this Linter addition.

TLDR:
no-undef: "warn" after we figure out how to instrument ESLint properly as to the file order/context
no-multi-spaces: "warn"
no-fallthrough: ["error", { "allowEmptyCase": true }]

EDIT: the flattened Switch statements, Revert all changes +
indent: ["error", "tab", {"SwitchCase": 1}]

EDIT2:
no-undef: "error"
no-implicit-globals: "error"

/* global ImprovedTube:writable */ just before this line https://github.com/dodieboy/YouTube-Extension/blob/fc970f93ff0c5fec793c2ef6923ef54299e6900b/js%26css/web-accessible/core.js#L15

/* global extension:writable */ just before this line https://github.com/dodieboy/YouTube-Extension/blob/fc970f93ff0c5fec793c2ef6923ef54299e6900b/js%26css/extension/core.js#L24

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

After reading thro all the changes only 39658f7 will require some reverts after applying ["error", "tab", {"SwitchCase": 1}], the rest looks great!

another rule to the pile:
no-trailing-spaces: "warn"

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

I think this 16531ca broke the Action?

[lint-and-test](https://github.com/code-charity/youtube/actions/runs/9788612176/job/27026963024#step:3:17)
Dependencies lock file is not found in /home/runner/work/youtube/youtube. Supported file patterns: package-lock.json,npm-shrinkwrap.json,yarn.lock
Set up Node.js
Run actions/setup-node@v4
Found in cache @ /opt/hostedtoolcache/node/20.15.0/x64
Environment details
/opt/hostedtoolcache/node/20.15.0/x64/bin/npm config get cache
/home/runner/.npm
Error: Dependencies lock file is not found in /home/runner/work/youtube/youtube. Supported file patterns: package-lock.json,npm-shrinkwrap.json,yarn.lock

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

I disable the "not defined" rule as there 3k+ error for undefined. Eslint check is done by looking at file extension .js or any others that you declare in the command and check anything is defined in that file order and import file only. I don't think there is anyway to declare the file order without import. The other way to fixed it is by declaring global in the config file as shown below
image
I will add "not defined" rule back after I added the global into the config.

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

I think this 16531ca broke the Action?

[lint-and-test](https://github.com/code-charity/youtube/actions/runs/9788612176/job/27026963024#step:3:17)
Dependencies lock file is not found in /home/runner/work/youtube/youtube. Supported file patterns: package-lock.json,npm-shrinkwrap.json,yarn.lock
Set up Node.js
Run actions/setup-node@v4
Found in cache @ /opt/hostedtoolcache/node/20.15.0/x64
Environment details
/opt/hostedtoolcache/node/20.15.0/x64/bin/npm config get cache
/home/runner/.npm
Error: Dependencies lock file is not found in /home/runner/work/youtube/youtube. Supported file patterns: package-lock.json,npm-shrinkwrap.json,yarn.lock

I will look into it later

@raszpl
Copy link
Contributor

raszpl commented Jul 4, 2024

I disable the "not defined" rule as there 3k+ error for undefined.

as expected :D

Eslint check is done by looking at file extension .js or any others that you declare in the command and check anything is defined in that file order and import file only. I don't think there is anyway to declare the file order without import.

means we indeed need those:

test_extension.js 
test_webaccessible.js
test_menu.js containing concatenated menu .js files, or does esling also take html files like menu/index.html? 

created and thrown into /test/ directory, and ESLint run only on 'background.js test_extension.js test_webaccessible.js test_menu.js' and nothing else

The other way to fixed it is by declaring global in the config file as shown below

saw that in your config (Im learning about configuring elsing as I go) and dont like it. Linter will still assume functions from other .js files are not defined in file being parsed at this moment etc.

I will add "not defined" rule back after I added the global into the config.

please dont :) better to massage eslint into interpreting all files in related bundles (menu/extension/webaccessible).

@dodieboy dodieboy deleted the ESlint branch July 4, 2024 11:23
@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

Added the new rule and refix the indent base on the new rule

@dodieboy
Copy link
Member Author

dodieboy commented Jul 4, 2024

Removed the cache function for now, when everything confirm already than I will add it back

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants