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

Unify/modernize code with eslint - use let, drop Node 12 support #2584

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

mvorisek
Copy link
Contributor

@mvorisek mvorisek commented Dec 4, 2022

let vs var declaration is safer, see https://stackoverflow.com/questions/762011/what-is-the-difference-between-let-and-var and is supported by every modern browser incl. IE11 - https://caniuse.com/?search=let

also Node 12 support is dropped as no longer supported by eslint-plugin-unicorn dep and it is EOL

also use rgb(... x%) instead of rgba(...)

@mvorisek mvorisek force-pushed the let_var_declaration branch 2 times, most recently from 5f5eec0 to c85fda9 Compare December 4, 2022 20:12
@mvorisek mvorisek marked this pull request as ready for review December 4, 2022 20:20
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 4, 2022

@lubber-de also what are your thoughts on using even const when the variables are unchanged?

@mvorisek mvorisek changed the title Replace "var" variable declarations with modern "let" Modernize code with autofixers part 1 Dec 5, 2022
@mvorisek mvorisek changed the title Modernize code with autofixers part 1 Unify/modernize code part 1 Dec 5, 2022
@mvorisek mvorisek changed the title Unify/modernize code part 1 Unify/modernize with eslint - use let, fix rare errors Dec 5, 2022
@mvorisek mvorisek changed the title Unify/modernize with eslint - use let, fix rare errors Unify/modernize code with eslint - use let, fix rare errors Dec 5, 2022
@mvorisek
Copy link
Contributor Author

mvorisek commented Dec 5, 2022

PR is done.

Most of the changes are done with autofix, so they can be reviewed by cherrypicking the .eslintrc.js config (and the added yarn deps) onto develop & run eslint autofix. The difference are manual changes.

@lubber-de
Copy link
Member

lubber-de commented Dec 5, 2022

Thanks. This has to wait until 2.10.x for the following reasons (i haven't reviewed yet!):

  • IE11 does not completely support let when used inside for loops afaik (and caniuse states that)
  • Dropping Node 12 support
  • Does not work correctly in Safari < 11 , Android 4.4 anymore (yes, we officially do not support them anymore, but we have at least currently stated in the readme, that FUI will basically still work)
  • Deep testing is needed, i suspect non-obvious bugs

However, i'll try to review as quick as possible 😉

@lubber-de lubber-de added this to the 2.10.x milestone Dec 5, 2022
@lubber-de lubber-de added type/feat Any feature requests or improvements type/ci Anything related to CI/CD lang/javascript Anything involving JavaScript labels Dec 5, 2022
@lubber-de
Copy link
Member

lubber-de commented Dec 5, 2022

@lubber-de also what are your thoughts on using even const when the variables are unchanged?

Fine for me. However: Same reasons as for let apply as mentioned above

@mvorisek mvorisek changed the title Unify/modernize code with eslint - use let, fix rare errors Unify/modernize code with eslint - use let, drop Node 12 support Dec 5, 2022
@lubber-de lubber-de added the tag/breaking-change Any pull request which is waiting for a breaking change release label Dec 5, 2022
@mvorisek mvorisek marked this pull request as draft December 6, 2022 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript tag/breaking-change Any pull request which is waiting for a breaking change release type/ci Anything related to CI/CD type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants