chore: use-either-for-composable-branching - #621
Conversation
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit fe83454
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #621 +/- ##
===========================================
+ Coverage 18.07% 67.38% +49.30%
===========================================
Files 155 23 -132
Lines 24398 2836 -21562
Branches 1203 500 -703
===========================================
- Hits 4410 1911 -2499
+ Misses 19988 925 -19063
🚀 New features to boost your workflow:
|
|
Deployed a0bc070 to https://ForgeRock.github.io/ping-javascript-sdk/pr-621/a0bc070f379af776506d56d7942ac87590a075de branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/journey-client - 92.6 KB (new) 📊 Minor Changes📉 @forgerock/davinci-client - 55.1 KB (-0.1 KB) ➖ No Changes➖ @forgerock/sdk-types - 9.1 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
657043d to
e1bfa59
Compare
e1bfa59 to
c487633
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
In some places, I can see that Either helps make the code more functional. But in other places I don't see the benefit of using Either over a simple Micro fail/succeed. When is it appropriate to use one over the other?
cd1eada to
3f447f6
Compare
9f7726a to
cd2d932
Compare
a74b40f to
c1dd76e
Compare
11254d6 to
3f746e2
Compare
1fbfd4b to
c377172
Compare
c377172 to
ba9740b
Compare
ba9740b to
fe83454
Compare
There was a problem hiding this comment.
Nx Cloud is proposing a fix for your failed CI:
We updated the type-level test assertions in updater-narrowing.types.test-d.ts to align with the PR's intentional Either pattern, where Fido collector updaters accept FidoRegistrationInputValue | GenericError and FidoAuthenticationInputValue | GenericError. The missing GenericError import was also added. This fixes the three toEqualTypeOf failures in the typecheck and test tasks.
Tip
✅ We verified this fix by re-running @forgerock/davinci-client:test, @forgerock/davinci-client:typecheck, @forgerock/davinci-suites:e2e-ci--src/phone-number-field.test.ts.
Suggested Fix changes
diff --git a/packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts b/packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
index 587f367..2037602 100644
--- a/packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
+++ b/packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts
@@ -7,6 +7,8 @@
/* eslint-disable @typescript-eslint/no-unused-vars */
import { describe, expectTypeOf, it } from 'vitest';
+import type { GenericError } from '@forgerock/sdk-types';
+
import type { Updater } from './client.types.js';
import type {
PasswordCollector,
@@ -193,8 +195,10 @@ describe('Updater Type Narrowing with Real Usage Pattern', () => {
const updater = mockUpdate(collector);
expectTypeOf(updater).toEqualTypeOf<Updater<FidoRegistrationCollector>>();
- // 3. The updater parameter should accept FidoRegistrationInputValue
- expectTypeOf(updater).parameter(0).toEqualTypeOf<FidoRegistrationInputValue>();
+ // 3. The updater parameter should accept FidoRegistrationInputValue | GenericError (Either)
+ expectTypeOf(updater)
+ .parameter(0)
+ .toEqualTypeOf<FidoRegistrationInputValue | GenericError>();
}
});
@@ -209,8 +213,10 @@ describe('Updater Type Narrowing with Real Usage Pattern', () => {
const updater = mockUpdate(collector);
expectTypeOf(updater).toEqualTypeOf<Updater<FidoAuthenticationCollector>>();
- // 3. The updater parameter should accept FidoAuthenticationInputValue
- expectTypeOf(updater).parameter(0).toEqualTypeOf<FidoAuthenticationInputValue>();
+ // 3. The updater parameter should accept FidoAuthenticationInputValue | GenericError (Either)
+ expectTypeOf(updater)
+ .parameter(0)
+ .toEqualTypeOf<FidoAuthenticationInputValue | GenericError>();
}
});
});
@@ -308,7 +314,9 @@ describe('Updater Type Narrowing with Real Usage Pattern', () => {
expectTypeOf(collector).toEqualTypeOf<FidoRegistrationCollector>();
const updater = mockUpdate(collector);
expectTypeOf(updater).toEqualTypeOf<Updater<FidoRegistrationCollector>>();
- expectTypeOf(updater).parameter(0).toEqualTypeOf<FidoRegistrationInputValue>();
+ expectTypeOf(updater)
+ .parameter(0)
+ .toEqualTypeOf<FidoRegistrationInputValue | GenericError>();
}
});
});
🔔 Heads up, your workspace has pending recommendations ↗ to auto-apply fixes for similar failures.
Or Apply changes locally with:
npx nx-cloud apply-locally WLP0-Akmg
Apply fix locally with your editor ↗ View interactive diff ↗
🎓 Learn more about Self-Healing CI on nx.dev
3 participants
JIRA Ticket
n/s
Description
look at what some either refactors could look like