feat(lit): make disabling dev mode warnings simpler #4901
Conversation
🦋 Changeset detectedLatest commit: 62f3191 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
cc @sorvell for review |
There was a problem hiding this comment.
We need to add a test for this. To facilitate this, let's consider adding to the issueWarning tracker the code for the warning in addition to the warning itself.
Then I hope you could do something like:
window.litIssuedWarnings ?? = new Set();
window.litIssuedWarnings.add(`dev-mode`);|
Done - though it won't work fully till lit/lit.dev#1350 is in place (which requires Lit team to update the dev website) Also, I noticed that reactive-element already disables warnings based on a code (which does not match the warning code): lit/packages/reactive-element/src/reactive-element.ts Lines 1168 to 1174 in 6a232e9 |
| @@ -97,15 +97,17 @@ let issueWarning: (code: string, warning: string) => void; | |||
| if (DEV_MODE) { | |||
| // Ensure warnings are issued only 1x, even if multiple versions of Lit | |||
| // are loaded. | |||
| const issuedWarnings: Set<string | undefined> = | |||
| (globalThis.litIssuedWarnings ??= new Set()); | |||
| globalThis.litIssuedWarnings ??= new Set(); | |||
There was a problem hiding this comment.
some of your tests are re-assigning globalThis.litIssuedWarnings.
to better support that use case, rather than having local issuedWarnings variable, always check what is the current globalThis.litIssuedWarnings value
| console.warn(warning); | ||
| issuedWarnings.add(warning); | ||
| globalThis.litIssuedWarnings!.add(warning); |
There was a problem hiding this comment.
While user now has the ability to disable warnings based on warning code, the issueWarning itself still only disables the warnings it emitted using warning content rather than code.
This is because, in case of reactive-element, the same warning message may be emitted once for each tag name (rather than once for entire app) - that seems like a useful behavior.
lit/packages/reactive-element/src/reactive-element.ts
Lines 1175 to 1178 in 6a232e9
| globalThis.litIssuedWarnings ??= new Set(); | ||
| globalThis.litIssuedWarnings!.add('dev-mode'); |
There was a problem hiding this comment.
dev-mode warning is now delayed by a microtask so we can disable the warning here (even after lit-element is already imported)
However, we can't do so inside of test/suiteSetup as they run later than a microtask.
Not an issue for my use case as disabling warnings in the global scope is now easy (so my library can do so for all consumers).
packages/reactive-element/src/test/reactive-element_dev_mode_disable_test.ts
Show resolved
Hide resolved
packages/lit-element/src/test/lit-element_dev_mode_disable_test.ts
Outdated
Show resolved
Hide resolved
packages/lit-element/src/test/lit-element_dev_mode_disable_test.ts
Outdated
Show resolved
Hide resolved
packages/reactive-element/src/test/reactive-element_dev_mode_disable_test.ts
Show resolved
Hide resolved
packages/reactive-element/src/test/reactive-element_dev_mode_disable_test.ts
Outdated
Show resolved
Hide resolved
|
Thanks for the review! Applied all suggested fixes |
|
Merge conflict resolved |
**Related Issue:** #10038 ## Summary 1. Don't log calcite version message in tests as it adds noise in tests and can even cause tests to fail (see #10038 for details) 2. Delay emission of a console message until a microtask to give users more time to disable the console message. This is the exact same change I implemented in Lit: lit/lit#4901 - see that pr and associated issue for reasoning - In case of calcite, the console message can be disabled by running `(globalThis as { calciteConfig?: { version?: string } }).calciteConfig ??= { version: " " };` before the microtask fires
Makes it much easier to work around #4877.
By adding queueMicrotask, the warnings are no longer emitted in the global scope, which makes disabling them much easier (see referenced issue for details).
With this change, I am able to add code for disabling this warning to my runtime code - with that, the warning is be disabled for all consumers of my library without them having to do anything special in their test setup files.
The warning is still enabled by default in vanilla Lit for those who prefer it.
The original discussion was only about the "Lit is in dev mode" warning, but I made this change for other global scope warnings too for consistency.
Testing
Verified that after this change I am able to silence Lit's dev mode warning regardless of the order of imports.