chore: type-exports #493
chore: type-exports #493
Conversation
|
WalkthroughRefines and expands public TypeScript type exports across davinci-, device-, journey-, and oidc-client packages, introduces generic Updater/CollectorValueType types, tightens updater generics in e2e components, adds a types test, and adjusts one package.json types export path. Changes
Sequence Diagram(s)No sequence diagram provided — changes are type/export surface and type refinements without control-flow or runtime feature additions. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 b160ecd
☁️ Nx Cloud last updated this comment at |
ryanbas21
force-pushed
the
update-type-exports
branch
from
eda8b6d to
ffcf876
Compare
| */ | ||
|
|
||
| // Re-export types from external dependencies that consumers need | ||
| export type { ConfigOptions } from '@forgerock/javascript-sdk'; |
There was a problem hiding this comment.
technically this package uses the javascript-sdk. we can update this later though.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #493 +/- ##
===========================================
+ Coverage 18.52% 67.21% +48.68%
===========================================
Files 138 97 -41
Lines 27402 7219 -20183
Branches 963 866 -97
===========================================
- Hits 5076 4852 -224
+ Misses 22326 2367 -19959
🚀 New features to boost your workflow:
|
@forgerock/davinci-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: |
|
Deployed 6915082 to https://ForgeRock.github.io/ping-javascript-sdk/pr-493/6915082429cdc2914ad886a44dfb280bfa18682b branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.2 KB, -100.0%) 📊 Minor Changes📈 @forgerock/device-client - 9.2 KB (+0.1 KB) ➖ No Changes➖ @forgerock/protect - 150.1 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/davinci-app/components/multi-value.ts (1)
9-20: Updater generic is correct; JSDoc still describes the wrong collector/behaviorThe new
updater: Updater<MultiSelectCollector>matches thestring[]valuesarray and theCollectorValueType<MultiSelectCollector>mapping, so the typing is solid. However, the JSDoc still referencesSingleSelectCollectorand “single-select behavior (like radio buttons)”, which doesn’t match theMultiSelectCollectorusage or the actual multi-select logic.You can align the docs with the implementation like this:
-/** - * Creates a group of checkboxes with single-select behavior (like radio buttons) +/** + * Creates a group of checkboxes with multi-select behavior * based on the provided data and attaches it to the form * @param {HTMLFormElement} formEl - The form element to attach the checkboxes to - * @param {SingleSelectCollector} collector - Contains the options and configuration - * @param {Updater} updater - Function to call when selection changes + * @param {MultiSelectCollector} collector - Contains the options and configuration + * @param {Updater<MultiSelectCollector>} updater - Function to call when selection changes */ export default function multiValueComponent( formEl: HTMLFormElement, collector: MultiSelectCollector, - updater: Updater<MultiSelectCollector>, + updater: Updater<MultiSelectCollector>, )
🧹 Nitpick comments (1)
e2e/davinci-app/components/object-value.ts (1)
56-56: Type assertions bypass type safety guarantees.The
as anycasts on lines 56 and 90 work around TypeScript's limitation with union function types but eliminate compile-time type checking at the call site. While this is a pragmatic solution given the union-typed updater parameter, it means type mismatches won't be caught until runtime.Consider these alternatives if type safety is a priority:
- Make the component function generic with a type parameter that ties the collector and updater types together
- Use separate handler functions for each collector type (already partially structured this way with the if/else branches)
- Use a discriminated approach where the updater is narrowed based on the collector type
Also applies to: 90-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/davinci-app/components/multi-value.ts(1 hunks)e2e/davinci-app/components/object-value.ts(3 hunks)e2e/davinci-app/components/password.ts(1 hunks)e2e/davinci-app/components/protect.ts(1 hunks)e2e/davinci-app/components/single-value.ts(1 hunks)e2e/davinci-app/components/text.ts(1 hunks)packages/davinci-client/src/lib/client.store.ts(1 hunks)packages/davinci-client/src/lib/client.types.ts(1 hunks)packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts(1 hunks)packages/davinci-client/src/types.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
e2e/davinci-app/components/protect.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (3)
Updater(21-21)TextCollector(42-42)ValidatedTextCollector(45-45)packages/davinci-client/src/lib/collector.types.ts (2)
TextCollector(184-184)ValidatedTextCollector(186-186)
packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts (3)
packages/davinci-client/src/types.ts (12)
PasswordCollector(41-41)TextCollector(42-42)ValidatedTextCollector(45-45)SingleSelectCollector(48-48)MultiSelectCollector(47-47)DeviceRegistrationCollector(49-49)DeviceAuthenticationCollector(50-50)PhoneNumberCollector(51-51)FidoRegistrationCollector(53-53)FidoAuthenticationCollector(54-54)Updater(21-21)Collectors(32-32)packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(297-300)FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
e2e/davinci-app/components/single-value.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (2)
Updater(21-21)SingleSelectCollector(48-48)packages/davinci-client/src/lib/collector.types.ts (1)
SingleSelectCollector(185-185)
e2e/davinci-app/components/multi-value.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (2)
Updater(21-21)MultiSelectCollector(47-47)packages/davinci-client/src/lib/collector.types.ts (1)
MultiSelectCollector(260-260)
e2e/davinci-app/components/password.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (2)
Updater(21-21)PasswordCollector(41-41)packages/davinci-client/src/lib/collector.types.ts (1)
PasswordCollector(183-183)
e2e/davinci-app/components/text.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (3)
Updater(21-21)TextCollector(42-42)ValidatedTextCollector(45-45)packages/davinci-client/src/lib/collector.types.ts (2)
TextCollector(184-184)ValidatedTextCollector(186-186)
packages/davinci-client/src/lib/client.types.ts (2)
packages/davinci-client/src/types.ts (2)
CollectorValueType(22-22)Updater(21-21)packages/davinci-client/src/lib/collector.types.ts (3)
PhoneNumberInputValue(297-300)FidoRegistrationInputValue(555-557)FidoAuthenticationInputValue(576-578)
e2e/davinci-app/components/object-value.ts (3)
packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)packages/davinci-client/src/types.ts (4)
Updater(21-21)DeviceRegistrationCollector(49-49)DeviceAuthenticationCollector(50-50)PhoneNumberCollector(51-51)packages/davinci-client/src/lib/collector.types.ts (3)
DeviceRegistrationCollector(402-405)DeviceAuthenticationCollector(406-409)PhoneNumberCollector(410-414)
packages/davinci-client/src/types.ts (2)
packages/davinci-client/src/lib/client.store.ts (1)
davinci(58-454)packages/davinci-client/src/lib/client.types.ts (2)
Updater(79-82)CollectorValueType(37-68)
packages/davinci-client/src/lib/client.store.ts (3)
packages/davinci-client/src/lib/collector.types.ts (4)
SingleValueCollectors(176-181)MultiSelectCollector(260-260)ObjectValueCollectors(390-395)AutoCollectors(647-652)packages/davinci-client/src/types.ts (2)
MultiSelectCollector(47-47)Updater(21-21)packages/davinci-client/src/lib/client.types.ts (1)
Updater(79-82)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pr
- GitHub Check: Mend Code Security Check
🔇 Additional comments (9)
packages/davinci-client/src/lib/client.types.ts (1)
25-68: Generic collector value mapping and Updater look correct and backward‑compatibleThe
CollectorValueType<T>conditional chain matches the known collector/value pairs and correctly falls back to the legacy value union, andUpdater<T>preserves previous behavior forT = unknownwhile enabling precise narrowing for specific collectors. This aligns well with the usages shown elsewhere in the PR.Also applies to: 70-82
e2e/davinci-app/components/single-value.ts (1)
15-19: Updater matches usage and collector mappingTyping
updaterasUpdater<SingleSelectCollector>aligns withCollectorValueType<SingleSelectCollector> = stringand theselectedValue: stringpassed from the change handler, with no runtime behavior changes.e2e/davinci-app/components/password.ts (1)
10-14: Password updater specialization is consistent with value type
updater: Updater<PasswordCollector>is consistent withCollectorValueType<PasswordCollector> = stringand the string value read from the input on blur, so the specialization is type‑safe without changing behavior.e2e/davinci-app/components/protect.ts (1)
13-17: Union-typed Updater<TextCollector | ValidatedTextCollector> is correctSpecializing
updaterasUpdater<TextCollector | ValidatedTextCollector>matches thestringliteral'fakeprofile'passed in and theCollectorValueTypemapping for both collector variants, improving type precision without affecting runtime behavior.e2e/davinci-app/components/text.ts (1)
18-18: LGTM! Proper type narrowing for updater parameter.The change from
UpdatertoUpdater<TextCollector | ValidatedTextCollector>correctly narrows the updater's type to match the collector parameter, leveraging the new genericUpdater<T>type. This improves type safety by ensuring the updater can only accept values compatible with these specific collector types.e2e/davinci-app/components/object-value.ts (1)
23-26: Union of function types necessitates runtime type assertions below.The updater parameter is now a union of
Updater<DeviceRegistrationCollector> | Updater<DeviceAuthenticationCollector> | Updater<PhoneNumberCollector>. When calling a union of functions in TypeScript, you can only pass arguments that satisfy all signatures in the union. Since these updaters expect different parameter types (stringvsPhoneNumberInputValue), calling the updater requires type assertions (see lines 56 and 90).packages/davinci-client/src/lib/client.store.ts (1)
237-245: Excellent type safety improvement with generic type parameter.The addition of the generic type parameter
Tto theupdatemethod enables proper type narrowing: when a specific collector type is passed, the returnedUpdater<T>is constrained to accept only values compatible with that collector type (viaCollectorValueType<T>). This provides compile-time guarantees that prevent passing incompatible values to updaters.packages/davinci-client/src/lib/updater-narrowing.types.test-d.ts (1)
1-297: Excellent compile-time type validation coverage.This type test file comprehensively validates that the generic
Updater<T>type correctly narrows based on collector types across various scenarios:
- Single value collectors (Password, Text, SingleSelect, DeviceRegistration, DeviceAuthentication)
- Multi value collectors (MultiSelect)
- Object value collectors (PhoneNumber, FidoRegistration, FidoAuthentication)
- Real-world usage patterns (forEach, for...of, early return)
- Edge cases (optional index parameter, complex conditionals)
The tests ensure that TypeScript's type narrowing works as expected when discriminating on
collector.type, which is critical for the type safety guarantees of this API.packages/davinci-client/src/types.ts (1)
16-16: Appropriate expansion of public API surface.The new exports enhance the public API by making key types available to consumers:
LogLevel(line 16): Enables proper logger configurationUpdater<T = unknown>(line 21): Generic variant for type-safe updater functions; default type parameter maintains backward compatibilityCollectorValueType<T>(line 22): Allows consumers to understand and work with the value types expected by different collectorsRequestMiddlewareandActionTypes(line 57): Enable request middleware configurationThese exports align with the PR objective of ensuring common types are re-exported.
Also applies to: 21-22, 57-57
JIRA Ticket
N/A release work
Description
Preliminary scan to make sure we are re-exporting common types.
Summary by CodeRabbit
Refactor
Tests