Skip to content

fix(jest-validate): Allow deprecation warnings for unknown options#14499

Merged
SimenB merged 9 commits intojestjs:mainfrom
dj-stormtrooper:fix/deprecation-warnings-for-unknown-options
Sep 11, 2023
Merged

fix(jest-validate): Allow deprecation warnings for unknown options#14499
SimenB merged 9 commits intojestjs:mainfrom
dj-stormtrooper:fix/deprecation-warnings-for-unknown-options

Conversation

@dj-stormtrooper
Copy link
Copy Markdown
Contributor

@dj-stormtrooper dj-stormtrooper commented Sep 7, 2023 •
edited
Loading

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
Copy Markdown

netlify Bot commented Sep 7, 2023 •
edited
Loading

✅ 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
Copy Markdown
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
Copy Markdown
Contributor Author

dj-stormtrooper commented Sep 8, 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?

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 dj-stormtrooper mentioned this pull request Sep 10, 2023
Merged
@dj-stormtrooper
Copy link
Copy Markdown
Contributor Author

dj-stormtrooper commented Sep 10, 2023 •
edited
Loading

@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

SimenB
SimenB approved these changes Sep 11, 2023
View reviewed changes
Copy link
Copy Markdown
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

Comment thread CHANGELOG.md Outdated

- `[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
Copy Markdown
Member

@SimenB SimenB Sep 11, 2023

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
Copy Markdown
Contributor Author

dj-stormtrooper commented Sep 11, 2023

Could you update its docs as well

Good idea, thanks, added the example of usage

SimenB
SimenB approved these changes Sep 11, 2023
View reviewed changes
Copy link
Copy Markdown
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
@SimenB
Copy link
Copy Markdown
Member

SimenB commented Sep 12, 2023

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

@ms-bot ms-bot mentioned this pull request Oct 2, 2023
Closed
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 13, 2023

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.

Reviewers

@SimenB SimenB SimenB approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@dj-stormtrooper @SimenB