feat(no-deprecated-functions): support jest version setting
#564
Conversation
|
@SimenB I think I might break |
G-Rath
force-pushed
the
support-version-setting
branch
from
90e7318
to
84499e3
Compare
G-Rath
force-pushed
the
support-version-setting
branch
from
84499e3
to
91ba077
Compare
| version: JestVersion; | ||
| } | ||
|
|
||
| const detectJestVersion = (): JestVersion => { |
There was a problem hiding this comment.
could we make this so that once we've found the version it's cached? so we don't do extra fs per test file.
For the test you might need jest.resetModules() so it'll be recalculated. Or move the detection to a separate file and just mock it 🙂
There was a problem hiding this comment.
For now I've just gone with a one line _clearCachedJestVersion function, as jest.resetModules didn't work, and so we'd have to have that function even if it was in another file.
I'll break it out into another file later :)
There was a problem hiding this comment.
you'd need to re-import the file after clearing so you don't hold on to a reference to the old module.
no biggie, tho
| } | ||
| } catch {} | ||
|
|
||
| throw new Error( |
There was a problem hiding this comment.
@SimenB I just remembered: I originally used toThrowErrorMatchingInlineSnapshot, but it kept complaining the snapshot was different everytime due to missing indentation at the start & end.
I'll try and put it into a reproduction 🐛
G-Rath
force-pushed
the
support-version-setting
branch
from
27e3251
to
b5e58e6
Compare
| ): Promise<string> => { | ||
| const jestPackageJson: JSONSchemaForNPMPackageJsonFiles = { | ||
| name: 'jest', | ||
| version: `${jestVersion}.0.0`, |
There was a problem hiding this comment.
we might add new "better" versions in minors, but this is fine for now 👍
# [23.10.0](v23.9.0...v23.10.0) (2020-05-09) ### Features * **no-deprecated-functions:** support jest `version` setting ([#564](#564)) ([05f20b8](05f20b8))
|
🎉 This PR is included in version 23.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
| @@ -9,10 +9,20 @@ of majors, eventually they are removed completely. | |||
| ## Rule details | |||
There was a problem hiding this comment.
Should this mention that it respects/varies based on jest version?
There was a problem hiding this comment.
You mean something like this?
(But could be improved by mentioning the actual setting, or at least linking back to that section in the readme)
There was a problem hiding this comment.
Linking back or something yeah, so people can be explicit
| @@ -22,13 +78,27 @@ export default createRule({ | |||
| }, | |||
| defaultOptions: [], | |||
| create(context) { | |||
| const jestVersion = | |||
| (context.settings as ContextSettings)?.jest?.version || | |||
There was a problem hiding this comment.
Can we do something so we don't need the type cast?
There was a problem hiding this comment.
No, as context.settings is just type object - it ideally should take a generic, but it's already two that are heavily inferred which means a generic would be unwieldy without support for a sigil to indicate partial type argumentation inference
(Actually technically "yes": we could do a typeguard, but that would be a subtle cast)
This is pretty cool, if I do say so myself.
For now while it's implemented all as util functions, I've lumped them in
no-deprecated-functionsasutilsis getting pretty big & I'd rather have this reviewed first since it's easy to move them out.I've tested this locally against a few projects, including
@typescript-eslint, and nothing went wrong.fixes #561