Skip to content

chore: enable eslint:no-shadow#4089

Merged
straker merged 7 commits intodevelopfrom
no-shadow
Jul 14, 2023
Merged

chore: enable eslint:no-shadow#4089
straker merged 7 commits intodevelopfrom
no-shadow

Conversation

@straker
Copy link
Copy Markdown
Contributor

@straker straker commented Jul 13, 2023

This came up in a recent pr so went ahead and did it (hurray for resolving some 111 issues). For every file I touched I did the following:

  • Moved default export to the top
  • Converted to es6 arrow functions and const / let
  • Refactored some improper uses of reduce to better array functions (when the reduce caused the name conflict)
  • Removed unneeded /* global */ eslint strings

In some files that had naming conflict issues the issues were in nested functions so I moved the function outside the nested scope.

@straker straker requested a review from a team as a code owner July 13, 2023 20:50
* @return {Mixed} True if aria-errormessage references an existing element that uses a supported technique. Undefined if it does not reference an existing element. False otherwise.
*/
function ariaErrormessageEvaluate(node, options, virtualNode) {
export default function ariaErrormessageEvaluate(node, options, virtualNode) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation behind this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follows our coding guidelines.

If you encounter any code that we maintain that does not put the default export at the top, you should update the file to do so.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation behind putting the default export at the top?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensures that when you open the file the main code path is the first thing you read.

Comment on lines +17 to +19
const ids = cells
.filter(cell => cell.getAttribute('id'))
.map(cell => cell.getAttribute('id'));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much easier to read/understand 👍

* }
*/

const mergeCheckLocale = (a, b) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - when is a function declaration preferred over a function expression? Also why is one preferred over the other?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know we have no standard for when to use either. Mostly it's been up to the person writing the code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I much prefer named functions, as they make debugging a bit easier.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both functions created using expressions and declarations have a name.

Welcome to Node.js v16.14.0.
Type ".help" for more information.
> function foo(){}
undefined
> foo.name
'foo'
> const bar = () => {}
undefined
> bar.name
'bar'
>

Co-authored-by: Stephen Mathieson <571265+stephenmathieson@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple suggestions.

* }
*/

const mergeCheckLocale = (a, b) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally I much prefer named functions, as they make debugging a bit easier.

straker and others added 2 commits July 14, 2023 07:52
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
@straker straker merged commit f151508 into develop Jul 14, 2023
@straker straker deleted the no-shadow branch July 14, 2023 16:41
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