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

fix: remove airbnb/google in style guides when using ts #33

Merged
merged 7 commits into from
Aug 8, 2022

Conversation

aladdin-add
Copy link
Member

as they do not officially support ts.
fixes #32

@eslint-github-bot eslint-github-bot bot added triage bug Something isn't working labels Jun 21, 2022
@@ -40,7 +40,8 @@
},
"mocha": {
"loader": "esmock",
"ui": "bdd"
"ui": "bdd",
"timeout": 10000
Copy link
Member Author

@aladdin-add aladdin-add Jun 21, 2022

Choose a reason for hiding this comment

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

just set it a bigger value - a test was sometimes failing as timeout in my local.

@aladdin-add aladdin-add force-pushed the chore/remove-airbnb-ts branch 2 times, most recently from 79d47ca to abbb3b2 Compare June 27, 2022 02:33
@mdjermanovic
Copy link
Member

Now we'll have the following situation: the latest eslint-config-standard-with-typescript v21.0.1 has "@typescript-eslint/parser": "^4.0.0" as a dependency, but when the user selects TypeScript, we're automatically adding the parser and the plugin regardless of the style guide, so at the end we'll install @typescript-eslint/parser@latest (currently v5).

√ How would you like to use ESLint? · style
√ What type of modules does your project use? · esm
√ Which framework does your project use? · none
√ Does your project use TypeScript? · No / Yes
√ Where does your code run? · browser
√ How would you like to define a style for your project? · guide
√ Which style guide do you want to follow? · standard-with-typescript
√ What format do you want your config file to be in? · JavaScript
Checking peerDependencies of eslint-config-standard-with-typescript@latest
Local ESLint installation not found.
The config that you've selected requires the following dependencies:

@typescript-eslint/eslint-plugin@^4.0.1 eslint-config-standard-with-typescript@latest eslint@^7.12.1 eslint-plugin-import@^2.22.1 eslint-plugin-node@^11.1.0 eslint-plugin-promise@^4.2.1 || ^5.0.0 typescript@^3.9 || ^4.0.0 @typescript-eslint/parser@latest

...

npm WARN @typescript-eslint/[email protected] requires a peer of @typescript-eslint/parser@^4.0.0 but none is installed. You must install peer dependencies yourself.

...

Maybe we should update our code to not automatically add @typescript-eslint packages to the list of packages that should be installed (and also not add references to them in the generated config file) when a style guide has been selected because this is already handled by the style guides?

@aladdin-add
Copy link
Member Author

aladdin-add commented Jul 5, 2022

should be addressed in 4b2e121. The logic was written a while ago, nowdays most plugins have a recommended preset, so it's no longer needed to add the parserOptions manually.

@mdjermanovic
Copy link
Member

Now, with the latest version of this PR, choosing eslint-config-xo-typescript does not install @typescript-eslint/parser. We're installing peer dependencies of configs, but for some reason eslint-config-xo-typescript does not have @typescript-eslint/parser in its peerDependencies.

@aladdin-add
Copy link
Member Author

but for some reason eslint-config-xo-typescript does not have @typescript-eslint/parser in its peerDependencies.

it has been added in eslint-config-xo-typescript v0.52.0: https://github.com/xojs/eslint-config-xo-typescript/releases/tag/v0.52.0

@aladdin-add aladdin-add force-pushed the chore/remove-airbnb-ts branch 5 times, most recently from b65f0ba to 9583e59 Compare July 25, 2022 05:38
@aladdin-add
Copy link
Member Author

friendly ping @mdjermanovic 😄

@mdjermanovic
Copy link
Member

Sorry for the delay, I'll take a look soon.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I'll leave this open for a few days in case someone else wants to review it before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug Something isn't working
Projects
None yet
2 participants