Skip to content

Integrate and remove dependency on Airbnb ESLint rules #11445

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

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Nov 1, 2024

🛠 Summary of changes

Updates ESLint configuration to integrate relevant rules from the Airbnb ESLint configurations eslint-config-airbnb and eslint-config-airbnb-base.

Why?

  • Incremental step toward migrating to ESLint 9 (updated configuration syntax not supported by Airbnb shared configurations)
  • More explicit control over rules relevant to our projects
    • No longer need to disable stylistic rules for Prettier compatibility
    • Avoid unnecessary configuration for tools and practices not relevant to our projects (e.g. in the course of this exercise, I discovered that Airbnb includes a number of configuration for React component classes, and other tools like Vue.js)
  • Shallower dependency tree, fewer dependencies to maintain
  • More honesty with how we communicate our JavaScript coding standards
    • We've claimed to follow the TTS JavaScript style recommendations, but we've so far diverged from the base configuration that this is mostly untrue. Keeping the dependency has largely been due to trying to maintain this claim more-so than actually needing it.

The intent of these changes is to largely keep an identical configuration to what existed previously. Minor revisions have been incorporated based on observations of the flattened configuration, reflected in the CHANGELOG.md.

These changes were implemented using ESLint's --print-config option to display the fully-resolved configuration, rearranging the result into relevant sub-configurations (TypeScript, Mocha, Prettier, etc.).

📜 Testing Plan

Verify that build passes.

Verify that lint passes:

yarn lint

changelog: Internal, Dependencies, Integrate and remove dependency on Airbnb ESLint rules
'import/no-cycle': [
'error',
{
maxDepth: '∞',
Copy link
Contributor

Choose a reason for hiding this comment

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

LOL at this being the unicode infinity instead of like of the JS Infinity constant, probably better JSON compat this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, hadn't noticed that 😅 I'd guess so? Though I could imagine the literal "Infinity" would work just as well too, and not require looking up the unicode character. 🙃

Comment on lines -56 to -59
Prettier works reasonably well in combination with Airbnb's JavaScript standards. In the few cases
where conflicts occur, formatting rules may be disabled to err toward Prettier conventions when an
option is not configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we state that our config is "inspired by Airbnb's" or have we diverged from that too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, yeah, I dunno that it's particularly useful to maintain the historical alignment or imply that we still seek to keep that alignment. In the future, I think we could also do to trim down the configuration a bit more, I just didn't want to change too much too quickly in this initial pull request.

@aduth aduth merged commit c2ce620 into main Nov 4, 2024
2 checks passed
@aduth aduth deleted the aduth-rm-eslint-airbnb branch November 4, 2024 12:02
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.

2 participants