Fix Metric.isMetric runtime brand - #6822
Conversation
🦋 Changeset detectedLatest commit: eca831d The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
isMetricnow checks the actualTypeIdconstant ("~effect/observability/Metric") instead of the stale string"~effect/Metric"- JSDoc example updated to show correct
true/falseresults - Test suite added covering constructed metrics, transformed metrics (
mapInput,withAttributes), and false-negatives including the old brand string - Patch changeset included
The fix is correct: Metric$ (line 1728) sets readonly [TypeId] = TypeId with TypeId = "~effect/observability/Metric" (line 1725), and all metric subclasses inherit this. The old "~effect/Metric" value was a leftover from a previous brand rename.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
There was a problem hiding this comment.
Pull request overview
This PR fixes Metric.isMetric to validate against the current internal Metric runtime brand so both newly constructed and transformed metrics are recognized correctly at runtime.
Changes:
- Update
Metric.isMetricimplementation to check the current Metric brand (TypeId) instead of the legacy string key. - Add focused unit tests covering constructed metrics, transformed metrics, and non-metric values.
- Add a patch changeset and simplify/correct the
isMetricJSDoc example.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/effect/test/Metric.test.ts | Adds coverage for Metric.isMetric across constructed/transformed/non-metric inputs. |
| packages/effect/src/Metric.ts | Updates Metric.isMetric runtime brand check and adjusts the JSDoc example accordingly. |
| .changeset/fix-metric-is-metric.md | Adds a patch changeset entry describing the runtime brand fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
-
Fixed
isMetrictype predicate variance — changed fromMetric<unknown, never>toMetric<never, unknown>, matching theMetric<in Input, out State>variance (Inputcontravariant →never;Statecovariant →unknown) -
Added type-level regression test — verifies
Metric.value(input)returnsEffect<unknown>after narrowing through the guard
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 04f645d:
- Removed dedicated
isMetricruntime tests — theit("isMetric")block inpackages/effect/test/Metric.test.tswas dropped. The JSDoc doctest coveringtruefor a real counter andfalsefor a plain object remains as the test surface. - Removed type-level test file —
packages/effect/typetest/Metric.tst.tswas deleted. The variance (Metric<never, unknown>) was already verified in the prior review and the fixedisMetricsource code is unchanged.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
Fixes
Metric.isMetricto check the current internal Metric brand so constructed and transformed metrics are recognized.Adds focused tests, a corrected JSDoc example, and a patch changeset.
Validation:
pnpm lint-fixgit diff --check