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

refactor: remove eslint-config-airbnb* dependencies #160

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

frantic1048
Copy link
Contributor

@frantic1048 frantic1048 commented Jul 1, 2024

This PR resolves the step 1 of #159

  1. Extracted the used eslint-config-airbnb* rules into our repository.
  2. No config snapshot changes

@frantic1048 frantic1048 self-assigned this Jul 1, 2024
@frantic1048 frantic1048 force-pushed the feature/remove-eslint-config-airbnb branch 10 times, most recently from 986c35e to d60fdcb Compare July 5, 2024 10:07
@frantic1048 frantic1048 requested review from lixiaoyan and liby July 5, 2024 10:22
@frantic1048
Copy link
Contributor Author

As @lixiaoyan suggests, this PR will be separated to more PRs for easier reviewing and smoother updates for downstream.

@frantic1048 frantic1048 force-pushed the feature/remove-eslint-config-airbnb branch 3 times, most recently from ec2488e to 9e19cad Compare July 8, 2024 08:55
@frantic1048 frantic1048 changed the title feat: remove eslint-config-airbnb* dependencies refactor: remove eslint-config-airbnb* dependencies Jul 8, 2024
@frantic1048 frantic1048 force-pushed the feature/remove-eslint-config-airbnb branch from 9e19cad to 0c68565 Compare July 8, 2024 09:04
@frantic1048 frantic1048 marked this pull request as ready for review July 8, 2024 09:07
Comment on lines +36 to +37
"@types/confusing-browser-globals": "1.0.3",
"@types/semver": "7.5.8"
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider moving these two dependencies to dependencies section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not reexporting these types; It is not very necessary to do so IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @liby What do you think ? 👀

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I would prefer to have the package and its type definition in the same section.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving type definition packages to dependencies can cause unnecessary bloat and version conflicts in production. It's generally better to keep them in devDependencies to avoid these issues, unless runtime type definitions are specifically needed. This keeps the dependency tree cleaner and more manageable.

Copy link
Contributor

Choose a reason for hiding this comment

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

After discussion, moving @types/* to dependencies does not have any obvious issues. This is more like a style preference issue:

  1. I prefer to put type packages that are not exported in devDependencies.

  2. I prefer to put the type package and its package together.

This reminds me of discussions about indentation, and if we want to choose a style preference, we can only vote within the entire team 🥹

@frantic1048 frantic1048 force-pushed the feature/remove-eslint-config-airbnb branch from 0c68565 to ef80176 Compare July 9, 2024 06:11
@frantic1048 frantic1048 requested a review from liby July 9, 2024 06:12
@frantic1048 frantic1048 merged commit 1dd301c into main Jul 9, 2024
6 checks passed
@frantic1048 frantic1048 mentioned this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants