-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(jest-validate): Allow deprecation warnings for unknown options #14499
fix(jest-validate): Allow deprecation warnings for unknown options #14499
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
If we log a deprecation warning, can we avoid also printing an "unrecognised" error? We'd still want to fail I guess, so maybe we need to allow some deprecations to also fail instead of just warning? |
Sounds good, I can see 2 options how to implement this:
Second one makes more sense to me |
@SimenB I updated the PR with If If Both scenarios are covered by unit tests |
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.
wonderful, thanks!
Could you update its docs as well? https://github.com/jestjs/jest/blob/main/packages/jest-validate/README.md
CHANGELOG.md
Outdated
@@ -8,6 +8,7 @@ | |||
|
|||
- `[jest-snapshot]` Allow for strings as well as template literals in inline snapshots ([#14465](https://github.com/jestjs/jest/pull/14465)) | |||
- `[@jest/test-sequencer]` Calculate test runtime if `perStats.duration` is missing ([#14473](https://github.com/jestjs/jest/pull/14473)) | |||
- `[jest-validate]` Allow deprecation warnings for unknown options ([#14499](https://github.com/jestjs/jest/pull/14499)) |
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.
should be features
I think
Good idea, thanks, added the example of usage |
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! Just a quick prettier fix in the readme and this should be good to go 👍
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
As discussed here: #14490 (comment)
Current deprecation logic is not flexible because
unrecognizedOptions
check runs before thedeprecation
check and we need to keep all deprecated options inallowedOptions
config. This PR allows printing deprecation notice for any options, which makes process of deprecation more simpleTest plan
I checked the behavior with
jest-cli
, passing the option which is not supported anymore (and therefore we don't have it inallowedOptions
)Then I added
collectCoverageOnlyFrom
to the allowed args and checked the other scenario: deprecated option is allowed in config (soft deprecation)Both scenarios are covered by unit test