-
Notifications
You must be signed in to change notification settings - Fork 3
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
3 new configurations added: /bun, /jsdoc, and /jsdoc-typescript #16
Conversation
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.
Some notes for context:
@@ -1,6 +1,6 @@ | |||
{ | |||
"plugins": ["eslint-plugin-playwright"], | |||
"extends": ["plugin:playwright/recommended", "./eslintNodeConfig.json", "eslint-config-prettier"], | |||
"extends": ["plugin:playwright/recommended"], |
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.
Node.js can no longer be assumed. Could be Bun or Node.
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.
eslint-config-prettier
only belongs as the last extends
entry, so it should only ever be in one of the environment configs:
- Node
- Bun
- Browser
@@ -3,14 +3,13 @@ | |||
"parserOptions": { | |||
"project": ["./tsconfig.json"] | |||
}, | |||
"plugins": ["@typescript-eslint/eslint-plugin", "eslint-plugin-import"], |
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.
eslint-plugin-import
is added in the base config
"eslint-config-airbnb-typescript/base", | ||
"plugin:@typescript-eslint/recommended-type-checked", | ||
"plugin:@typescript-eslint/stylistic-type-checked", | ||
"./eslintBaseConfig.json", | ||
"eslint-config-prettier" | ||
"plugin:import/typescript" |
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.
adding to eslint-plugin-import
should only happen after it is first added to extends
(in eslintBaseConfig.json
)
"author": "Eric L. Goldstein <[email protected]>", | ||
"description": "Hierarchical ESLint configuration collection that intends to be simple to use, layered, and shared with others", | ||
"keywords": [ | ||
"eslint", | ||
"eslintconfig" | ||
], | ||
"engines": { | ||
"node": ">=16.13.0" | ||
"node": "^18.18.0 || >=20.0.0" |
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.
semver breaking change
}, | ||
"peerDependencies": { | ||
"eslint": ">=8.44.0" | ||
"eslint": "^8.56.0" |
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.
semver breaking change (version bump, and eslint 9 is not allowed due to airbnb incompatibility)
@@ -1,4 +1,4 @@ | |||
{ | |||
"root": true, | |||
"extends": ["../../lib/eslintPlaywrightConfig.json"] | |||
"extends": ["../../lib/eslintPlaywrightConfig.json", "../../lib/eslintNodeConfig.json"] |
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.
required for test to pass
|
||
// ASTRO BASE TSCONFIG | ||
// ASTRO BASE TSCONFIG (https://github.com/withastro/astro/blob/main/packages/astro/tsconfigs/base.json) |
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.
here down is a copy of all Astro's tsconfigs
"verbatimModuleSyntax": true, | ||
"jsx": "preserve", | ||
"allowJs": false, | ||
"module": "Preserve", |
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.
Bun-compatible module mode (import
and require
in the same file)
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.
Thanks a lot for the added configs and the package updates 👏
I added two minor comments, I think the only one we really need to look into is the postinstall
script as I think its implications are unintended, otherwise ready to go!
@@ -1,5 +1,17 @@ | |||
# Changelog | |||
|
|||
## 2.0.0 |
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 we could highlight the breaking changes here for ease of update?
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.
Sure thing, adding
@@ -39,10 +38,9 @@ | |||
{ | |||
"groups": [ | |||
["builtin", "external"], | |||
["index", "internal", "parent", "sibling"], | |||
["index", "internal", "parent", "sibling", "unknown"], |
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.
Changes in this file don't seem to be documented in the Changelog
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.
Added
package.json
Outdated
"validate:formatting": "prettier --check --no-editorconfig .", | ||
"validate:linting:eslint": "eslint-config-prettier ./lib/eslintBaseConfig.json" | ||
"list:todo-comments": "rg --only-matching '(TODO|FIXME)([\t ]+\\[[^\\]]+\\])?:[\\w\\s!\"#$%&'\\''()+,./:;<=>?@\\^_`{|}~-]+'", | ||
"postinstall": "bun run --silent check:environment:versions", |
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.
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.
Oh no, I forgot about that. Thank you for pointing that out! I was intending to ensure the proper Bun version was used for maintaining this library.
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.
I tried a few ways to keep it in, but I ended up removing it
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.
Could you have forgotten to push? I don't seem to see any changes
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.
I pushed to origin
(my fork) not upstream
. I just pushed now. Thanks for pointing that out.
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.
Thanks for the changes!
Looking great 🚀
Pull Request Checklist
package.json
following Semantic VersioningChanges Included
2.0.0
18.18.0
is now the minimum supported runtime version because of dependency requirements8.56.0
is now the minimum supported version9.x
cannot be supported yet due to a dependency on theeslint-config-airbnb*
packages and its strict peer dependency requirement of no higher than ESLint8.x
/bun
for Bun support/jsdoc
for JSDoc support in JavaScript projects/jsdoc-typescript
for JSDoc support in TypeScript projectsInternal Imports
module imports grouping