-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Warn on use of plugin parameters as function #14661
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
Warn on use of plugin parameters as function #14661
Conversation
| test('theme keys can read from the CSS theme', () => { | ||
| let theme = new Theme() | ||
| theme.add('--color-green', 'green') | ||
| let originalWarn = console.warn |
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.
I'd probably just mock console for the entire test file — it'll help cleanup the test and makes adding / testing additional warnings in the future simpler:
const warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined);
afterAll(() => warn.mockReset()); expect(warn).toHaveBeenCalledWith(
'Using the plugin object parameter as the theme function is deprecated. Please use the `theme` property instead.',
)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.
Not a fan of mocking apis like this too broadly since it can lead to us hiding warnings somewhere where we don't want it. Can put this test into a describe block to scope this and still use the same pattern though 👍 (or can use the onTestFinished hook to clean up and have it inside the single test only.. 🤔)
| let warn = vi.spyOn(console, 'warn').mockImplementation(() => undefined) | ||
| onTestFinished(() => { | ||
| warn.mockReset() | ||
| }) |
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.
ooh I forgot this existed 💯
This reverts commit a64e209.
Quick follow-up to #14659 base don @thecrypticace's idea: