Skip to content
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

Conversation

dj-stormtrooper
Copy link
Contributor

@dj-stormtrooper dj-stormtrooper commented Sep 7, 2023

Summary

As discussed here: #14490 (comment)

Current deprecation logic is not flexible because unrecognizedOptions check runs before the deprecation check and we need to keep all deprecated options in allowedOptions config. This PR allows printing deprecation notice for any options, which makes process of deprecation more simple

Test plan

I checked the behavior with jest-cli, passing the option which is not supported anymore (and therefore we don't have it in allowedOptions)

image

Then I added collectCoverageOnlyFrom to the allowed args and checked the other scenario: deprecated option is allowed in config (soft deprecation)

image

Both scenarios are covered by unit test

@netlify
Copy link

netlify bot commented Sep 7, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 573aae3
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/64ff511155739b0008a96fa4
😎 Deploy Preview https://deploy-preview-14499--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SimenB
Copy link
Member

SimenB commented Sep 7, 2023

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?

@dj-stormtrooper
Copy link
Contributor Author

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:

  1. Add field error: bolean or warning: boolean to Deprecated.ts . This will be a breaking change for jest-config because current API of these entries is (options) => string
  2. Always throw an error on deprecation check if argument is not presented in the allowedOptions list

Second one makes more sense to me

@dj-stormtrooper
Copy link
Contributor Author

dj-stormtrooper commented Sep 10, 2023

@SimenB I updated the PR with fatal behavior (without any breaking changes), see the example:


If collectCoverageOnlyFrom option is removed from the allowed args list:

image

If collectCoverageOnlyFrom is still in the allowed args list (soft deprecation that allow the usage)

image

Both scenarios are covered by unit tests

Copy link
Member

@SimenB SimenB left a 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))
Copy link
Member

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

@dj-stormtrooper
Copy link
Contributor Author

Could you update its docs as well

Good idea, thanks, added the example of usage

Copy link
Member

@SimenB SimenB left a 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 👍

@SimenB SimenB merged commit f0cfd50 into jestjs:main Sep 11, 2023
90 of 91 checks passed
@SimenB
Copy link
Member

SimenB commented Sep 12, 2023

https://github.com/jestjs/jest/releases/tag/v29.7.0

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants