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

Bump eslint to version v8 #952

Closed
wants to merge 0 commits into from

Conversation

gulderov
Copy link
Contributor

@gulderov gulderov commented Aug 9, 2023

Description

Please review eslint update to version 8.46.0

The primary intent of this PR is to update tooling. No functionality or logic has been altered

Changes:

  • Bump eslint from 7.10.0 to 8.46.0
  • Bump prettier from 2.1.2 to 3.0.1
  • eslint --fix and sass-lint to auto fix new warnings
  • Suppressed linter warnings to maintain consistency in this pull request
  • prettier config ajustments
    • Removed jsxBracketSameLine in favor of bracketSameLine option
    • Omitted theparser configuration as prettier is able to autodetect
    • Defaulted to standard values for jsxSingleQuote, semi and printWith

Issues Resolved

Check List

  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

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

LGTM, we just need to get CI passing. Not sure why puppeteer is throwing a fit all of the sudden. I can't see anything that might have changed it

Comment on lines 238 to 240
const { field: sortable, direction: sortDirection } = (
sorting as SortingOptions
).sort;
Copy link
Contributor

Choose a reason for hiding this comment

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

Feel like I personally prefer the old version of this, but we can follow up on code style in another task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel the same way. I'm not a big fan of Prettier, but it's used in the project regardless.

Copy link
Contributor

@BSFishy BSFishy left a comment

Choose a reason for hiding this comment

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

CI is passing :D

@@ -12,19 +12,19 @@
// This file uses RGBA literal values responsibly
// This file uses off-pattern indentation to be more readable

// sass-lint:disable no-color-literals, no-color-keywords, indentation, quotes
// sass-lint:disable no-color-literals, no-color-keywords, indentation, quotes, hex-length hex-notation leading-zero
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if there was really a need to disable "hex-length", "hex-notation", "leading-zero"? I've also noticed this in other files as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They would trigger sass-lint errors if not muted

Comment on lines 41 to 48
/* eslint-disable import/named */
import {
FixedSizeList,
ListProps,
ListChildComponentProps as ReactWindowListChildComponentProps,
areEqual,
} from 'react-window';
/* eslint-enable import/named */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we avoid these disabling comments, maybe we can add some rules to eslint config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue should be resolved by the typings for react-window, which is out of the scope of this pull request. Personally, I prefer to avoid making changes to the eslint config and to keep it as limited as possible

@BSFishy
Copy link
Contributor

BSFishy commented Sep 20, 2023

It seems like there are build errors caused by maybe a prettier api change? Seems to be an issue on these lines:

const prettierOptions = await prettier.resolveConfig(extractedVarTypesFilename);
const prettifiedModuleSource = prettier.format(moduleSource, prettierOptions);

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

Successfully merging this pull request may close these issues.

3 participants