-
-
Notifications
You must be signed in to change notification settings - Fork 21
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: correct defineConfig
usage in generated config
#161
base: main
Are you sure you want to change the base?
Conversation
lib/config-generator.js
Outdated
|
||
this.result.configContent = `${importContent} | ||
${needCompatHelper ? helperContent : ""} | ||
export default defineConfig([\n${exportContent || " {}\n"}]);`; // defaults to `[{}]` to avoid empty config warning | ||
export default ${isDefineConfigExported ? `defineConfig(${exportContent})` : exportContent};`; // defaults to `[{}]` to avoid empty config warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach won't work because we are using the extends
key elsewhere, which requires defineConfig()
.
Instead, if defineConfig
isn't an export of the eslint
package, we should install @eslint/config-helpers
and import it from there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated 👍🏻
lib/config-generator.js
Outdated
* - "9.22" or higher versions without prefix | ||
* - Correctly handles full semver notation (e.g., 9.22.0, ~9.22.0) | ||
*/ | ||
const ESLINT_9_PLUS_REGEX = /\^9|>=\s*9|(?:~|^)9\.(2[2-9]|[3-9]\d|\d{3,})/u; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would make sense to use the semver
package instead? That way we could eliminate one source of bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we can use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated 👍🏻
@@ -119,7 +120,8 @@ export class ConfigGenerator { | |||
const languages = this.answers.languages ?? ["javascript"]; | |||
const purpose = this.answers.purpose; | |||
|
|||
let importContent = "import { defineConfig } from \"eslint/config\";\n"; | |||
let isDefineConfigExported = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it make sense to change to the default to true
, as:
create-config/lib/config-generator.js
Line 83 in 886f39e
devDependencies: ["eslint"], |
so the only thing is to change it to false
when [email protected] is not allowed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending answer to @aladdin-add's question.
@snitin315 your feedback is wanted here. :) |
Fix #160