Conversation
|
📝 WalkthroughWalkthroughThe pull request introduces a new bundle size verification system for monitoring SDK feature bundles. It adds end-to-end test fixtures for multiple ForgeRock SDKs, a Rollup-based bundling and measurement script, a dedicated E2E app configuration, and CI/CD workflow steps to automatically generate and report bundle size comparisons on pull requests. ChangesBundle Size Verification System
Sequence DiagramsequenceDiagram
participant GH as GitHub Action
participant FS as File System
participant RB as Rollup Bundler
participant SZ as Size Calculator
participant CM as Comment Manager
participant PR as Pull Request
GH->>FS: Discover fixture files<br/>(*.ts in fixtures/)
loop For each fixture
GH->>RB: Bundle with Rollup<br/>(node-resolve, esbuild, terser)
RB->>FS: Write minified output
RB->>SZ: Emit bundle code
SZ->>SZ: Compress with gzip level 9
SZ->>FS: Record minified & gzip sizes
end
GH->>FS: Load previous baseline<br/>(if available)
SZ->>SZ: Calculate size deltas<br/>vs. baseline
SZ->>FS: Write baseline.json<br/>(fixture name → sizes)
SZ->>FS: Generate Markdown table<br/>(fixture, minified, gzip, delta)
GH->>CM: Export report to outputs
CM->>PR: Find existing bundle comment
alt Comment exists
CM->>PR: Update with latest report
else No comment
CM->>PR: Create new comment
end
PR-->>GH: Display tree-shaken bundle sizes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes The changes introduce a cohesive bundle-checking feature with consistent fixture patterns (18 similar files reduce cognitive load) and moderate-complexity bundling logic in Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 4da1672
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (17.61%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #627 +/- ##
===========================================
- Coverage 70.90% 17.61% -53.29%
===========================================
Files 53 154 +101
Lines 2021 24243 +22222
Branches 377 1160 +783
===========================================
+ Hits 1433 4271 +2838
- Misses 588 19972 +19384 🚀 New features to boost your workflow:
|
@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: |
|
Deployed 06531c0 to https://ForgeRock.github.io/ping-javascript-sdk/pr-627/06531c0c9fd9d3c1f0832b11e506253af9f32fae 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%) ➖ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In @.github/workflows/publish.yml:
- Around line 99-104: The upload-artifact step named "Upload feature bundle
baseline" currently relies on the default if-no-files-found behavior (warn)
which can let the job pass when
e2e/bundle-check-app/dist/bundle-feature-baseline.json is missing; update the
actions/upload-artifact@v5 step (the block with name: Upload feature bundle
baseline and name: bundle-feature-baseline) to include the input
if-no-files-found: error so the workflow fails fast when the baseline JSON is
not present.
In `@e2e/bundle-check-app/fixtures/effect-subpath-option.ts`:
- Around line 3-11: The test fixture currently imports the entire Option
namespace which prevents precise tree-shaking; replace the namespace import with
explicit named imports for the used symbols (fromNullable and match) by updating
the import of Option to import { fromNullable, match } from 'effect/Option' and
then update usages in the pipe (Option.fromNullable → fromNullable, Option.match
→ match) so Rollup can accurately include only those symbols.
In `@e2e/bundle-check-app/fixtures/journey-client.ts`:
- Around line 9-15: The post-loop if is dead because the while loop only exits
when step.type !== 'Step', so client.redirect can never run; fix by detecting
redirect-flavored steps inside the loop (e.g., check for step.type ===
'Redirect' or the specific redirect variant) and call client.redirect(step)
there (then break/return), or alternatively change the loop condition so it
stops before a redirect step so the outer if can fire; update the logic around
step, client.next and client.redirect accordingly.
In `@e2e/bundle-check-app/fixtures/journey-full.ts`:
- Around line 35-39: The current conditional after the loop can never hit the
else-if branch because the while loop exits only when step.type !== 'Step';
update the control flow so RedirectCallback-bearing Step responses are handled:
inside the loop (or immediately after receiving a new step) check for step.type
=== 'Step' and call client.redirect(step) when that Step contains a
RedirectCallback, or reorder the post-loop checks to inspect the actual
callbacks on step before assuming LoginSuccess; specifically adjust the logic
around step.type, the while loop that consumes Step responses, and the
client.redirect(step) invocation so RedirectCallback flows are exercised
(references: step.type, client.redirect, LoginSuccess, Step).
In `@e2e/bundle-check-app/package.json`:
- Around line 32-37: The package.json "inputs" array for the bundle-check-app
target is only tracking "{workspaceRoot}/packages/journey-client/dist/**/*.js"
and misses built-artifact globs for the other workspace deps; update the
"inputs" array to include the dist globs for the remaining packages (e.g.,
"{workspaceRoot}/packages/davinci-client/dist/**/*.js",
"{workspaceRoot}/packages/device-client/dist/**/*.js",
"{workspaceRoot}/packages/oidc-client/dist/**/*.js",
"{workspaceRoot}/packages/protect/dist/**/*.js") so NX will invalidate cache
when those built outputs change; ensure you modify the same "inputs" list in the
file to add these entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1a851b96-0d0b-4699-9fab-ed635d5500e7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.github/workflows/ci.yml.github/workflows/publish.ymle2e/bundle-check-app/fixtures/davinci-client-flow.tse2e/bundle-check-app/fixtures/davinci-client.tse2e/bundle-check-app/fixtures/device-client.tse2e/bundle-check-app/fixtures/effect-barrel-option.tse2e/bundle-check-app/fixtures/effect-subpath-option.tse2e/bundle-check-app/fixtures/journey-client-device.tse2e/bundle-check-app/fixtures/journey-client-policy.tse2e/bundle-check-app/fixtures/journey-client-qr-code.tse2e/bundle-check-app/fixtures/journey-client-recovery-codes.tse2e/bundle-check-app/fixtures/journey-client-webauthn.tse2e/bundle-check-app/fixtures/journey-client.tse2e/bundle-check-app/fixtures/journey-full.tse2e/bundle-check-app/fixtures/oidc-client-logout.tse2e/bundle-check-app/fixtures/oidc-client-token.tse2e/bundle-check-app/fixtures/oidc-client.tse2e/bundle-check-app/fixtures/protect.tse2e/bundle-check-app/fixtures/redirect.tse2e/bundle-check-app/package.jsone2e/bundle-check-app/src/bundle.tse2e/bundle-check-app/tsconfig.jsontsconfig.json
| - name: Upload feature bundle baseline | ||
| uses: actions/upload-artifact@v5 | ||
| with: | ||
| name: bundle-feature-baseline | ||
| path: e2e/bundle-check-app/dist/bundle-feature-baseline.json | ||
| retention-days: 30 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For actions/upload-artifact@v5, what is the default behavior of if-no-files-found, and does it fail the workflow by default?
💡 Result:
The default behavior of if-no-files-found for actions/upload-artifact@v5 is 'warn', which outputs a warning but does not fail the action or the workflow.[1][2][3][4] It does not fail the workflow by default; the action succeeds with a warning when no files are found.[1][5]
Citations:
- 1: https://github.com/actions/upload-artifact/tree/refs/heads/upload-artifact-v6-release
- 2: https://github.com/actions/upload-artifact/blob/v4/README.md
- 3: https://github.com/actions/upload-artifact/tree/v4.2.0
- 4: https://www.github.com/actions/upload-artifact
- 5: Add new option to specify behavior if no files found actions/upload-artifact#104
Add if-no-files-found: error to fail fast when the feature baseline file is missing.
The default if-no-files-found behavior is warn, which allows the action to succeed silently if the baseline JSON is absent. This breaks the CI invariant that the baseline must exist for downstream comparisons. Setting it to error ensures the workflow fails immediately if the file is not generated.
Proposed change
- name: Upload feature bundle baseline
uses: actions/upload-artifact@v5
with:
name: bundle-feature-baseline
path: e2e/bundle-check-app/dist/bundle-feature-baseline.json
+ if-no-files-found: error
retention-days: 30🤖 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 @.github/workflows/publish.yml around lines 99 - 104, The upload-artifact
step named "Upload feature bundle baseline" currently relies on the default
if-no-files-found behavior (warn) which can let the job pass when
e2e/bundle-check-app/dist/bundle-feature-baseline.json is missing; update the
actions/upload-artifact@v5 step (the block with name: Upload feature bundle
baseline and name: bundle-feature-baseline) to include the input
if-no-files-found: error so the workflow fails fast when the baseline JSON is
not present.
| import * as Option from 'effect/Option'; | ||
| import { pipe } from 'effect/Function'; | ||
|
|
||
| const result = pipe( | ||
| Option.fromNullable(Math.random() > 0.5 ? 'hello' : null), | ||
| Option.match({ | ||
| onNone: () => 'none', | ||
| onSome: (v) => v.toUpperCase(), | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Rollup tree-shaking, does import * as X from "module"generally retain more code thanimport { a, b } from "module"when onlyaandb are used?
💡 Result:
Yes, in Rollup tree-shaking, import * as X from "module" generally retains more code than import { a, b } from "module" when only a and b are used.[1][2][3] Rollup's tree-shaking analyzes static ES module imports to include only used exports.[4][1] Named imports like { a, b } explicitly mark only those exports as used, allowing Rollup to eliminate unused code from "module".[1] Namespace imports (import * as X) create a namespace object containing all exports.[5][2] While recent improvements (e.g., PR #6230) enable partial tree-shaking by including only accessed properties like X.a and X.b in some cases,[2] it generally retains more code than named imports for several reasons: - Namespace creation may include additional logic or properties, especially with re-exports or dynamic access.[2][6] - Property access X.a can trigger side-effect assumptions (e.g., getters), potentially preserving more.[7][8] - Historical bugs and complexities with namespaces (e.g., issues #1951, #4751) show less optimal shaking compared to direct named imports.[9][10] - General bundler behavior and StackOverflow consensus note namespace imports hinder optimal tree-shaking due to dynamic-like access.[3][11] Official docs emphasize named imports for minimal bundles.[1] For best tree-shaking when using only specific exports, prefer named imports.[1][2]
Citations:
- 1: https://rollupjs.org/introduction/
- 2: Refine namespace handling rollup/rollup#6230
- 3: https://stackoverflow.com/questions/75699675/javascript-does-the-use-of-namespace-imports-have-an-effect-on-a-modules-tree
- 4: https://rollupjs.org/faqs
- 5: https://rollupjs.org/configuration-options/
- 6: Include all paths of reexports if namespace is used rollup/rollup#5837
- 7: rollup/rollup@5ca7216
- 8: Add new treeshaking options rollup/rollup#1760
- 9: Named Imports Incorrectly removed if import * is also used rollup/rollup#1951
- 10: incorrect tree shaking with
import * as foo+export default foorollup/rollup#4751 - 11: re-exported namespaces cannot be tree-shaked evanw/esbuild#1420
🏁 Script executed:
# Check if the file exists and view its current content
cat -n e2e/bundle-check-app/fixtures/effect-subpath-option.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 559
🏁 Script executed:
# List other fixture files to see if there's a pattern
ls -la e2e/bundle-check-app/fixtures/Repository: ForgeRock/ping-javascript-sdk
Length of output: 1376
🏁 Script executed:
cat -n e2e/bundle-check-app/fixtures/effect-barrel-option.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 556
Use named imports to ensure accurate tree-shaking measurements for this subpath fixture.
The file's comment states it tests whether Rollup tree-shakes to only fromNullable and match. Using import * as Option creates a namespace object, which Rollup's tree-shaker retains more code for compared to explicit named imports, making this fixture less reliable for measuring subpath import effectiveness. Named imports allow the bundler to precisely identify and include only the used symbols.
Proposed change
-import * as Option from 'effect/Option';
+import { fromNullable, match } from 'effect/Option';
import { pipe } from 'effect/Function';
const result = pipe(
- Option.fromNullable(Math.random() > 0.5 ? 'hello' : null),
- Option.match({
+ fromNullable(Math.random() > 0.5 ? 'hello' : null),
+ match({
onNone: () => 'none',
onSome: (v) => v.toUpperCase(),
}),
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import * as Option from 'effect/Option'; | |
| import { pipe } from 'effect/Function'; | |
| const result = pipe( | |
| Option.fromNullable(Math.random() > 0.5 ? 'hello' : null), | |
| Option.match({ | |
| onNone: () => 'none', | |
| onSome: (v) => v.toUpperCase(), | |
| }), | |
| import { fromNullable, match } from 'effect/Option'; | |
| import { pipe } from 'effect/Function'; | |
| const result = pipe( | |
| fromNullable(Math.random() > 0.5 ? 'hello' : null), | |
| match({ | |
| onNone: () => 'none', | |
| onSome: (v) => v.toUpperCase(), | |
| }), |
🤖 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 `@e2e/bundle-check-app/fixtures/effect-subpath-option.ts` around lines 3 - 11,
The test fixture currently imports the entire Option namespace which prevents
precise tree-shaking; replace the namespace import with explicit named imports
for the used symbols (fromNullable and match) by updating the import of Option
to import { fromNullable, match } from 'effect/Option' and then update usages in
the pipe (Option.fromNullable → fromNullable, Option.match → match) so Rollup
can accurately include only those symbols.
| while (step.type === 'Step') { | ||
| step = await client.next(step); | ||
| } | ||
|
|
||
| if (step.type === 'Step') { | ||
| await client.redirect(step); | ||
| } |
There was a problem hiding this comment.
Dead code: redirect block can never execute after the while loop
The while (step.type === 'Step') loop only exits when step.type !== 'Step', making the if (step.type === 'Step') guard on Line 13 permanently false. client.redirect will never be called, so the bundler tree-shakes it out entirely — defeating the fixture's stated purpose of measuring redirect's bundle cost.
The check should be inside the loop (e.g., on a redirect-flavoured step type) or the loop condition needs to stop before the redirect step so the outer check can fire:
🐛 Proposed fix
-while (step.type === 'Step') {
- step = await client.next(step);
-}
-
-if (step.type === 'Step') {
- await client.redirect(step);
-}
+while (step.type === 'Step') {
+ if (/* step signals a redirect */ false) {
+ await client.redirect(step);
+ break;
+ }
+ step = await client.next(step);
+}Alternatively, if redirect is meant to be called on a non-Step result type (e.g., 'Redirect'), update the condition accordingly:
-if (step.type === 'Step') {
+if (step.type === 'Redirect') {
await client.redirect(step);
}🤖 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 `@e2e/bundle-check-app/fixtures/journey-client.ts` around lines 9 - 15, The
post-loop if is dead because the while loop only exits when step.type !==
'Step', so client.redirect can never run; fix by detecting redirect-flavored
steps inside the loop (e.g., check for step.type === 'Redirect' or the specific
redirect variant) and call client.redirect(step) there (then break/return), or
alternatively change the loop condition so it stops before a redirect step so
the outer if can fire; update the logic around step, client.next and
client.redirect accordingly.
| if (step.type === 'LoginSuccess') { | ||
| console.log('authenticated', step.getSessionToken()); | ||
| } else if (step.type === 'Step') { | ||
| await client.redirect(step); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the current @forgerock/journey-clientAPI, what concrete step type(s) can be returned by start()/next() that should be handled via client.redirect(step), and what is the recommended branching pattern after awhile (step.type === 'Step') loop?
💡 Result:
Steps returned by start/next with type 'Step' that contain RedirectCallback(s) should be handled via client.redirect(step). This occurs, for example, after selecting an external IdP in a SelectIdPCallback, where the server responds with a RedirectCallback containing the IdP's authentication endpoint.[1][2] The recommended branching pattern after the while (step.type === 'Step') loop checks for 'LoginSuccess' (retrieve session token via step.getSessionToken) or 'LoginFailure' (display error via step.getMessage or step.payload.message), with an else for unexpected types.[2][3]
Citations:
- 1: https://developer.pingidentity.com/orchsdks/journey/use-cases/external-idp/javascript/02_authenticate_with_external_idps.html
- 2: https://www.npmjs.com/package/@forgerock/journey-client
- 3: https://developer.pingidentity.com/orchsdks/journey/usage/javascript/05-navigating-an-authentication-journey.html
Redirect branch is unreachable after the loop.
The while (step.type === 'Step') loop exits only when step.type !== 'Step', making the else if (step.type === 'Step') condition impossible to reach. This means client.redirect(step) never executes in this fixture, even though the API returns Step-type objects containing RedirectCallback(s) that require this handling (e.g., after selecting an external IdP). The fixture cannot exercise the full intended journey path.
🤖 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 `@e2e/bundle-check-app/fixtures/journey-full.ts` around lines 35 - 39, The
current conditional after the loop can never hit the else-if branch because the
while loop exits only when step.type !== 'Step'; update the control flow so
RedirectCallback-bearing Step responses are handled: inside the loop (or
immediately after receiving a new step) check for step.type === 'Step' and call
client.redirect(step) when that Step contains a RedirectCallback, or reorder the
post-loop checks to inspect the actual callbacks on step before assuming
LoginSuccess; specifically adjust the logic around step.type, the while loop
that consumes Step responses, and the client.redirect(step) invocation so
RedirectCallback flows are exercised (references: step.type, client.redirect,
LoginSuccess, Step).
| "inputs": [ | ||
| "{projectRoot}/src/**/*.ts", | ||
| "{projectRoot}/fixtures/**/*.ts", | ||
| "{projectRoot}/bundle-feature-baseline.json", | ||
| "{workspaceRoot}/packages/journey-client/dist/**/*.js" | ||
| ], |
There was a problem hiding this comment.
NX inputs missing dist globs for 4 of 5 workspace dependencies
Only packages/journey-client/dist/**/*.js is tracked. Changes to the built artifacts of davinci-client, device-client, oidc-client, and protect won't invalidate the NX cache, so the bundle-check target can return stale measurements after those packages are rebuilt.
🔧 Proposed fix
"inputs": [
"{projectRoot}/src/**/*.ts",
"{projectRoot}/fixtures/**/*.ts",
"{projectRoot}/bundle-feature-baseline.json",
- "{workspaceRoot}/packages/journey-client/dist/**/*.js"
+ "{workspaceRoot}/packages/journey-client/dist/**/*.js",
+ "{workspaceRoot}/packages/davinci-client/dist/**/*.js",
+ "{workspaceRoot}/packages/device-client/dist/**/*.js",
+ "{workspaceRoot}/packages/oidc-client/dist/**/*.js",
+ "{workspaceRoot}/packages/protect/dist/**/*.js"
],🤖 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 `@e2e/bundle-check-app/package.json` around lines 32 - 37, The package.json
"inputs" array for the bundle-check-app target is only tracking
"{workspaceRoot}/packages/journey-client/dist/**/*.js" and misses built-artifact
globs for the other workspace deps; update the "inputs" array to include the
dist globs for the remaining packages (e.g.,
"{workspaceRoot}/packages/davinci-client/dist/**/*.js",
"{workspaceRoot}/packages/device-client/dist/**/*.js",
"{workspaceRoot}/packages/oidc-client/dist/**/*.js",
"{workspaceRoot}/packages/protect/dist/**/*.js") so NX will invalidate cache
when those built outputs change; ensure you modify the same "inputs" list in the
file to add these entries.
2 participants
JIRA Ticket
N/A
Description
Wrote a small app that imports from
fixtures. Fixtures are small examples of our sdk in usage (not real for customers) that we can use to analyze bundle impact. We use rollup + terser + tree shaking. This way we can have real measurements of the code and how it gets removed.This will help us identify impact of things like
effector other tools that should tree shake themselves that we are removing as best we can.That way if we pull something we can visually see its impact.
This will post a comment on the PR, and on merges to main will save the current main branches state. then do comparisons from
mainto theprso its more identifiableSummary by CodeRabbit
New Features
Tests
Chores