-
Notifications
You must be signed in to change notification settings - Fork 37
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: ESLint 7.8.0 removes normalization function #332
fix: ESLint 7.8.0 removes normalization function #332
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.
The challenge here might be that the new dependency might not work on older eslint versions or in older node versions.
I can change it so eslintrc is the final fallback, instead of throwing error. Would it be better to release this as a breaking change with major version. I can go back and see if the logic changes in older version of eslint but I feel like it should be good from eslint v6 or v5. |
Breaking changes are never better :-) Having it as a fallback is fine, as long as we keep the current logic for eslint < v7.8. |
@ljharb I added back the old logic and created a new case for version 7. I felt like new case would work best since it wouldn't have to fail each case before falling back to use eslintrc naming functions. Since we can't use the require('eslint/....') to detect eslint version I just imported the new ESLint class that has the version number as a static property. |
Push another fix to remove object deconstruction to support nodejs v4. |
Got the change passing now!. Had to make @eslint/eslintrc be conditionally required since it is not compatible with nodejs v4. |
feefa52
to
bde54d6
Compare
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! I added explicit eslint v7.7 tests, and confirmed that if we ran any tests on master right now, they'd fail - which proves that this PR also fixes them.
normalizePackageName: naming.normalizePackageName, | ||
getShorthandName: naming.getShorthandName | ||
} | ||
}, | ||
// eslint >= 6.1.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.
// eslint >= 6.1.0 | |
// eslint 6.1.0 - 7.7.x |
Turns out this was a breaking change, because the eslintrc dep has an engines requirement of node 10 or 12+, and this package does not declare an engines requirement (but we tested on node 4+). Not sure how to address it. |
Looks like this will work fine if we just remove the explicit dep. I'll do this in #338. |
Issue #331
ESLint 7.8.0 removed the normalization functions this package used for package/plugin naming. The naming functions have now been moved into @eslint/eslintrc package. This change includes eslintrc as a dependency and removes the old logic of finding the function in the eslint source code.