useObserver registry refactor - #3598
Conversation
🦋 Changeset detectedLatest commit: b6e7128 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
mweststrate
left a comment
mweststrate
left a comment
There was a problem hiding this comment.
Looking good, thanks for putting that in a separate PR!
| throw new Error("This test must run with node >= 14") | ||
| } | ||
|
|
||
| expect(observerFinalizationRegistry).toBeInstanceOf(globalThis.FinalizationRegistry) |
There was a problem hiding this comment.
What does it do if it fails? it's not inside any test
| import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" | ||
| import { TimerBasedFinalizationRegistry } from "../src/utils/UniversalFinalizationRegistry" | ||
|
|
||
| expect(observerFinalizationRegistry).toBeInstanceOf(TimerBasedFinalizationRegistry) |
There was a problem hiding this comment.
What does it do if it fails?
Throws?
it's not inside any test
Does it matter?
It's mainly intented to assert environment (bug in the test), rather than testing that observerFinalizationRegistry resolves to correct type (bug in the actual tested code).
Arguably I could split these into two, assertion to check globalThis for (not) being FinalizationRegistry and then an actual test for observerFinalizationRegistry. However I don't see much value in doing so.
| import gc from "expose-gc/function" | ||
| import { observerFinalizationRegistry } from "../src/utils/observerFinalizationRegistry" | ||
|
|
||
| if (typeof globalThis.FinalizationRegistry !== "function") { |
There was a problem hiding this comment.
Maybe change this assertion also to expect to match the other checks?
There was a problem hiding this comment.
... But then the error message won't be the same think think
There was a problem hiding this comment.
Yes, I did want to preserve the message.
4 participants
As discussed here #3590 (comment)