docs(journey-client): fix FRDevice mapping, export PolicyParams, improve migration guide#630
docs(journey-client): fix FRDevice mapping, export PolicyParams, improve migration guide#630vatsalparikh wants to merge 1 commit into
Conversation
…ove migration guide
🦋 Changeset detectedLatest commit: 698f24e The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 |
📝 WalkthroughWalkthroughThis PR adds ChangesExport Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
vatsalparikh
requested review from
SteinGabriel,
ancheetah,
cerebrl and
ryanbas21
|
View your CI Pipeline Execution ↗ for commit 698f24e
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
MIGRATION.md (1)
194-244: 💤 Low valueComprehensive guide for creating test mocks.
The examples clearly demonstrate the shift from class instantiation to typed object literals. The code is syntactically correct and the imports align with the updated module structure.
One consideration: The
getCallbackOfTypeimplementation at line 218 throws an error with "not implemented". While this is appropriate for a minimal mock example in tests/stories, consider adding a brief note that production code or more complete test fixtures would need full implementations of all methods.📝 Suggested clarification
Add a note after line 225 or in the introduction paragraph at line 196:
const mockStep: JourneyStep = { type: StepType.Step, payload: rawPayload, callbacks: rawPayload.callbacks?.map(createCallback) ?? [], getCallbackOfType: (type) => { throw new Error('not implemented'); }, getCallbacksOfType: (type) => [], setCallbackValue: () => {}, getDescription: () => undefined, getHeader: () => undefined, getStage: () => undefined, }; + +// Note: This is a minimal mock suitable for stories and basic tests. +// Production code or comprehensive test fixtures should implement all methods fully.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@MIGRATION.md` around lines 194 - 244, Add a short clarifying note near the mock examples (around the mockStep/mockSuccess/mockFailure block) explaining that the provided JourneyStep/JourneyLoginSuccess/JourneyLoginFailure object literals are minimal test stubs and that getCallbackOfType currently throws ("not implemented") as a placeholder; recommend implementing full method behavior (e.g., getCallbackOfType, getCallbacksOfType, setCallbackValue, etc.) for production code or more comprehensive test fixtures so readers know when to replace the placeholder with real logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@MIGRATION.md`:
- Around line 194-244: Add a short clarifying note near the mock examples
(around the mockStep/mockSuccess/mockFailure block) explaining that the provided
JourneyStep/JourneyLoginSuccess/JourneyLoginFailure object literals are minimal
test stubs and that getCallbackOfType currently throws ("not implemented") as a
placeholder; recommend implementing full method behavior (e.g.,
getCallbackOfType, getCallbacksOfType, setCallbackValue, etc.) for production
code or more comprehensive test fixtures so readers know when to replace the
placeholder with real logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f1b1fb-0a8d-4757-9c8a-27f0793e0d1e
📒 Files selected for processing (7)
.changeset/cool-kings-learn.mdMIGRATION.mdinterface_mapping.mdpackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/src/index.tspackages/journey-client/src/types.ts
💤 Files with no reviewable changes (1)
- packages/journey-client/src/index.ts
@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
❌ 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 #630 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24242 +22221
Branches 377 1160 +783
===========================================
+ Hits 1433 4271 +2838
- Misses 588 19971 +19383
🚀 New features to boost your workflow:
|
|
Deployed 36ff687 to https://ForgeRock.github.io/ping-javascript-sdk/pr-630/36ff68711289ad3906aa362fc0fc8eff74183ce1 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/device-client - 0.0 KB (-10.0 KB, -100.0%) 📊 Minor Changes📉 @forgerock/journey-client - 91.8 KB (-0.1 KB) ➖ No Changes➖ @forgerock/device-client - 10.0 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
| export * from './types.js'; | ||
|
|
||
| // Re-export types from internal packages that consumers need | ||
| export { callbackType } from '@forgerock/sdk-types'; |
There was a problem hiding this comment.
this is tricky since its both a type and a value. we may want to keep this here, doing otherwise breaks the api.
There was a problem hiding this comment.
Oh okay. Moving it to types looked cleaner. Why does it break the API? Can we
export type { callbackType } from '@forgerock/sdk-types';
along with
export { callbackType, PolicyKey, StepType } from '@forgerock/sdk-types';
in types.ts file? Just a thought to keep index.ts cleaner.
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4796
Description
PolicyParamsfrom@forgerock/journey-client/types(was only accessible via@forgerock/sdk-types)callbackTypere-export fromindex.tsintotypes.tsalongside other value exports — no consumer impactFRDevicemapping ininterface_mapping.md: was incorrectly pointing todeviceClientfrom@forgerock/device-client; it maps toDevicefrom@forgerock/journey-client/deviceMIGRATION.md:StepOptionstoStartParam/NextOptions/ResumeOptions— covers the three implementation pain points not obvious from the type tablenew FRStep()/new FRLoginSuccess()/new FRLoginFailure()with typed object literals for mocks and storiesDid you add a changeset?
Yes — patch for
@forgerock/journey-client(newPolicyParamsexport).Summary by CodeRabbit
New Features
PolicyParamstype is now exported from journey-client/types for enhanced type support.Documentation