Conversation
|
|
||
| // Use bracket notation to avoid Webpack static analysis flagging missing exports | ||
| // This is important for Effect v3 compatibility. | ||
| const MetricModule = Metric; |
There was a problem hiding this comment.
This is a little hack I had to do in order to support Effect v3 in the browser. It seems that Webpack checks if snapshotUnsafe is available and fails if it doesn't. snapshotUnsafe is for Effect v4 and is not part of the package. By using MetricModule it is possible to work around this issue.
JPeer264
force-pushed
the
jp/effect-v4-core
branch
from
0ddfb1b to
0f6b4de
Compare
size-limit report 📦
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0f6b4de. Configure here.
s1gr1d
left a comment
s1gr1d
left a comment
There was a problem hiding this comment.
Looks good 👍
Just a general comment: when trying to differentiate the logic between v3 and v4 you always check for the structure of specific objects. If e.g. in v4 cause.reasons is always an array of Reason objects, you could theoretically just do something like isV4() and then apply the logic. But I am not sure if there is an easy way of finding out the used version (maybe checking the package.json)
I thought for it to make it more generic, but since we do not have any bundler plugin or similar there is no chance to check for it. Effect itself doesn't have a versioning internally, which makes it harder. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const V3_COUNTER_STATE_TYPE_ID = Symbol.for('effect/MetricState/Counter'); | ||
| const V3_GAUGE_STATE_TYPE_ID = Symbol.for('effect/MetricState/Gauge'); | ||
| const V3_HISTOGRAM_STATE_TYPE_ID = Symbol.for('effect/MetricState/Histogram'); | ||
| const V3_SUMMARY_STATE_TYPE_ID = Symbol.for('effect/MetricState/Summary'); | ||
| const V3_FREQUENCY_STATE_TYPE_ID = Symbol.for('effect/MetricState/Frequency'); |
There was a problem hiding this comment.
Bug: The hardcoded symbols used for Effect v3 metric detection are untested. If they are incorrect, v3 metrics will be silently dropped.
Severity: HIGH
Suggested Fix
Add an E2E test case for the effect-node (v3) application that specifically sends and verifies metrics. This will validate that the hardcoded symbol keys are correct and that metrics are being reported as expected for Effect v3 users.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: packages/effect/src/metrics.ts#L39-L43
Potential issue: The code uses hardcoded, globally-registered symbols (e.g.,
`Symbol.for('effect/MetricState/Counter')`) to detect Effect v3 metric types. This logic
is not covered by any unit or E2E tests, as the test suite only runs against Effect v4.
If these symbol keys do not exactly match the internal symbols used by Effect v3, the
`sendV3MetricToSentry` function will fail to identify any metrics, causing them to be
silently dropped without any errors or warnings.
Also affects:
packages/effect/src/metrics.ts:71~94
Did we get this right? 👍 / 👎 to inform future reviews.
2 participants
This adds support to Effect v4, but also keeps the compatibility for v3. There is no way that we can unit test against v3, as the
devDependenciesneed to useeffect@4and an updated@effect/vitestversion, which is not compatible with Effect v3 (this is added in #20389).The API for Effect v4 has changed a little, so there are safeguards to detect if it is v3 or v4 and uses the correct API. The good part is that for users nothing changed, so they still can use the same methods in their app as before (ofc, respecting the new Effect v4 API).
Before (Effect v3):
After (Effect v4):
Both usages still work and are represented in the E2E tests.