Conversation
|
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
📝 WalkthroughWalkthroughAdds an e2e bundle-check app with Rollup-based bundling and size measurement fixtures, integrates CI steps to run the bundle check and upload/download a baseline artifact, and posts/updates a PR comment showing tree-shaken feature bundle sizes and deltas versus baseline. 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 (*.ts)
loop For each fixture
GH->>RB: Bundle fixture (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 (if available)
SZ->>SZ: Compute size deltas vs baseline
SZ->>FS: Write baseline JSON and Markdown report
GH->>CM: Export report to workflow 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
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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 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 1c62a75
☁️ 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 f8dda8e to https://ForgeRock.github.io/ping-javascript-sdk/pr-627/f8dda8e8f594b3ba8b22c61b44eb5d4ca5bab715 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.
| "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.
ryanbas21
force-pushed
the
bundle-size-app-checker
branch
from
4da1672 to
1c62a75
Compare
Tree-shaken Feature Bundle SizesMinified + gzip level-9 cost of each SDK feature in isolation, as a consumer would receive it.
How these are measuredEach fixture imports a single feature and is bundled with Rollup + esbuild + terser (full minification, ESM, tree-shaking on). Numbers reflect what a consumer ships and what their users download, not the raw dist size. Updated automatically on each push to this PR |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/publish.yml (1)
100-100: 💤 Low value
actions/upload-artifact@v5is outdated — v6 and v7 are now available.v5 had preliminary support for Node.js 24 but still ran on Node.js 20 by default; v6 onward defaults to Node.js 24. The same pin exists at line 90 for the baseline step, so both would need to be bumped together for consistency.
🤖 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 at line 100, Replace the outdated "uses: actions/upload-artifact@v5" entries with a newer release (preferably `@v7`) so the action defaults to Node.js 24; update both occurrences of the exact string "uses: actions/upload-artifact@v5" in the workflow to "uses: actions/upload-artifact@v7" to keep them consistent.e2e/bundle-check-app/fixtures/oidc-client-logout.ts (1)
15-20: ⚡ Quick winKeep the logout fixture scoped to one feature.
client.token.revoke()pulls revocation code into the same bundle, so the reportedoidc-client-logoutsize is no longer attributable to logout alone. Split revoke into its own fixture, or rename this fixture so the combined measurement is explicit.🤖 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/oidc-client-logout.ts` around lines 15 - 20, The fixture mixes two features — token revocation and end-session logout — causing the bundle size measurement for oidc-client-logout to include revocation code; split the revoke call into its own fixture or rename the current fixture to reflect both actions: move the call to client.token.revoke() (and any imports it pulls in) into a separate fixture file (e.g., oidc-client-revoke) or remove it from this file so this fixture only calls client.logout(); update test/fixture names accordingly so the bundle-size measurement reflects a single feature (reference client.token.revoke() and client.logout()).
🤖 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 `@e2e/bundle-check-app/src/bundle.ts`:
- Around line 122-127: The current try/catch around reading and parsing
prevBaselinePath swallows all errors (including JSON parse and permission
errors) causing broken baselines to be treated as first-run; change the catch in
the block that imports node:fs and sets prevBaseline to only suppress
missing-file errors by inspecting the caught error (e.g., check err.code ===
'ENOENT' or err.code === 'ENOTDIR') and rethrow any other errors so
parse/permission issues surface; update references to prevBaseline and
prevBaselinePath accordingly so only a true "file not found" falls through to
the "first run" behavior.
- Around line 86-95: The generated bundle can emit multiple chunks when
dependencies use dynamic imports; update the Rollup generate call
(bundle.generate) to pass inlineDynamicImports: true so Rollup inlines dynamic
imports and produces a single deterministic chunk before you close the bundle
and write the file; modify the bundle.generate invocation to include
inlineDynamicImports: true alongside format: 'esm'.
---
Nitpick comments:
In @.github/workflows/publish.yml:
- Line 100: Replace the outdated "uses: actions/upload-artifact@v5" entries with
a newer release (preferably `@v7`) so the action defaults to Node.js 24; update
both occurrences of the exact string "uses: actions/upload-artifact@v5" in the
workflow to "uses: actions/upload-artifact@v7" to keep them consistent.
In `@e2e/bundle-check-app/fixtures/oidc-client-logout.ts`:
- Around line 15-20: The fixture mixes two features — token revocation and
end-session logout — causing the bundle size measurement for oidc-client-logout
to include revocation code; split the revoke call into its own fixture or rename
the current fixture to reflect both actions: move the call to
client.token.revoke() (and any imports it pulls in) into a separate fixture file
(e.g., oidc-client-revoke) or remove it from this file so this fixture only
calls client.logout(); update test/fixture names accordingly so the bundle-size
measurement reflects a single feature (reference client.token.revoke() and
client.logout()).
🪄 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: 567af2bf-4eb8-43bc-bf83-319600b2512f
⛔ 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
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/bundle-check-app/fixtures/oidc-client-token.ts
| const { output } = await bundle.generate({ format: 'esm' }); | ||
| await bundle.close(); | ||
|
|
||
| const code = output | ||
| .filter((chunk) => chunk.type === 'chunk') | ||
| .map((chunk) => chunk.code) | ||
| .join(''); | ||
|
|
||
| // Write minified output for inspection | ||
| writeFileSync(path.join(outDir, `${name}.min.js`), code, 'utf8'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n e2e/bundle-check-app/src/bundle.ts | head -120Repository: ForgeRock/ping-javascript-sdk
Length of output: 4750
🏁 Script executed:
cat -n e2e/bundle-check-app/package.jsonRepository: ForgeRock/ping-javascript-sdk
Length of output: 1631
🏁 Script executed:
grep -r "bundle.generate" e2e/bundle-check-app --include="*.ts" --include="*.js"Repository: ForgeRock/ping-javascript-sdk
Length of output: 171
🏁 Script executed:
web_search
Rollup inlineDynamicImports default behavior version 4.59Repository: ForgeRock/ping-javascript-sdk
Length of output: 169
🏁 Script executed:
find e2e/bundle-check-app/fixtures -name "*.ts" -type fRepository: ForgeRock/ping-javascript-sdk
Length of output: 941
🌐 Web query:
Rollup 4.59 inlineDynamicImports default behavior documentation
💡 Result:
Rollup’s output.inlineDynamicImports default behavior is to NOT inline dynamic imports. - output.inlineDynamicImports (CLI: --inlineDynamicImports / --no-inlineDynamicImports) is a boolean option with default false. [1] - When true, Rollup will inline dynamic imports instead of creating new chunks (only possible if a single input is provided). [1] - Turning it on can change execution order: a module that is only dynamically imported will be executed immediately if the dynamic import is inlined. [1] Notes for “Rollup 4.59”: the referenced Rollup 4.59.1 change mentions bug fixes related to lazy dynamic imports with moduleSideEffects:false, but it doesn’t change the documented default for inlineDynamicImports (still false per the option docs). [2]
Citations:
🏁 Script executed:
head -30 e2e/bundle-check-app/fixtures/journey-client.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 425
🏁 Script executed:
grep -r "import(" e2e/bundle-check-app/fixtures/ --include="*.ts"Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
cat -n e2e/bundle-check-app/src/bundle.ts | sed -n '84,102p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 659
🏁 Script executed:
# Check if any fixtures use top-level await or other split-inducing features
grep -r "dynamic import\|import.meta\|top-level await" e2e/bundle-check-app/fixtures/ --include="*.ts" | head -20Repository: ForgeRock/ping-javascript-sdk
Length of output: 55
🏁 Script executed:
cat -n e2e/bundle-check-app/src/bundle.ts | sed -n '85,102p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 646
🏁 Script executed:
# Check if any asset imports or other features might cause code splitting
cat e2e/bundle-check-app/fixtures/journey-client.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 425
🏁 Script executed:
# Verify the complete set of all fixtures to ensure none have dynamic imports
find e2e/bundle-check-app/fixtures -name "*.ts" -exec grep -l "async\|await\|import\|export" {} \; | wc -lRepository: ForgeRock/ping-javascript-sdk
Length of output: 76
🏁 Script executed:
cat -n e2e/bundle-check-app/src/bundle.ts | sed -n '71,84p'Repository: ForgeRock/ping-javascript-sdk
Length of output: 726
🌐 Web query:
Rollup multiple chunks single input no dynamic imports ESM output
💡 Result:
To get “roll up multiple chunks into a single input” with an ESM output in Rollup, you generally cannot with multiple inputs: you must provide a single input entry point and inline dynamic imports. 1) Single input + ESM + inline dynamic imports (single JS file) Rollup’s option output.inlineDynamicImports “inlines dynamic imports instead of creating new chunks to create a single bundle” but it is “only possible if a single input is provided” [1]. Use: - output.format: 'es' - output.file: 'dist/bundle.js' (single file) - output.inlineDynamicImports: true Example rollup.config.js: export default { input: 'src/index.js', output: { file: 'dist/bundle.js', format: 'es', inlineDynamicImports: true } }; This will produce a single ESM bundle, even when your code uses dynamic import (Rollup will inline it instead of emitting separate chunk files) [1]. 2) No dynamic imports: you still may get multiple chunks (because static graph splitting) If your goal is “no dynamic imports” but you’re still seeing many chunk files, that’s because Rollup may decide to split the graph into multiple chunks for reasons other than dynamic import (e.g., multiple entry points / code-splitting behavior). Rollup’s chunking rules are tied to the entry point setup and whether multiple chunks are needed [1]. 3) Multiple inputs + “single bundle” is not supported via inlineDynamicImports If you try inlineDynamicImports with multiple inputs, Rollup rejects it: “multiple inputs are not supported when output.inlineDynamicImports is true” [2]. So if you have many “chunks” because you modeled many things as multiple entry points, the Rollup-native way to keep a single output file is: - create a single entry module that imports everything you need, and use that as the one (and only) Rollup input. Pattern: // src/index.js export * from './chunk-a.js'; export * from './chunk-b.js'; Then build with input: 'src/index.js' and output.inlineDynamicImports: true (harmless if you truly have no dynamic imports). 4) If you’re using Vite/Rollup under the hood The same Rollup constraint applies: inlineDynamicImports can be used to remove extra chunk files, but it’s contingent on having a single input [1]. (People often work around multi-entry needs by building each entry separately, e.g., one Rollup build per entry with inlineDynamicImports true) [3]. Key constraints to remember - Single output file via inlineDynamicImports only works with a single input entry point [1]. - With multiple inputs, you’ll either get multiple outputs/chunks or you must restructure to a single input module that statically pulls in everything.
Citations:
- 1: https://rollupjs.org/configuration-options
- 2: https://github.com/rollup/rollup/blob/master/src/utils/options/normalizeOutputOptions.ts
- 3: Support multiple inputs when "output.inlineDynamicImports" is true. rollup/rollup#5601
🏁 Script executed:
# Check if there are any explicit output settings in rollup config
grep -A5 "generate\|output" e2e/bundle-check-app/src/bundle.tsRepository: ForgeRock/ping-javascript-sdk
Length of output: 474
Ensure single-chunk output by inlining dynamic imports.
Rollup's output.inlineDynamicImports defaults to false, which causes it to emit separate chunks for any dynamic imports in dependencies. While these fixtures don't directly use dynamic imports, external packages could, resulting in multiple chunks being concatenated into the single .min.js file. Adding inlineDynamicImports: true ensures deterministic single-bundle output regardless of what the dependency graph contains.
Suggested fix
- const { output } = await bundle.generate({ format: 'esm' });
+ const { output } = await bundle.generate({
+ format: 'esm',
+ inlineDynamicImports: true,
+ });📝 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.
| const { output } = await bundle.generate({ format: 'esm' }); | |
| await bundle.close(); | |
| const code = output | |
| .filter((chunk) => chunk.type === 'chunk') | |
| .map((chunk) => chunk.code) | |
| .join(''); | |
| // Write minified output for inspection | |
| writeFileSync(path.join(outDir, `${name}.min.js`), code, 'utf8'); | |
| const { output } = await bundle.generate({ | |
| format: 'esm', | |
| inlineDynamicImports: true, | |
| }); | |
| await bundle.close(); | |
| const code = output | |
| .filter((chunk) => chunk.type === 'chunk') | |
| .map((chunk) => chunk.code) | |
| .join(''); | |
| // Write minified output for inspection | |
| writeFileSync(path.join(outDir, `${name}.min.js`), code, 'utf8'); |
🤖 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/src/bundle.ts` around lines 86 - 95, The generated
bundle can emit multiple chunks when dependencies use dynamic imports; update
the Rollup generate call (bundle.generate) to pass inlineDynamicImports: true so
Rollup inlines dynamic imports and produces a single deterministic chunk before
you close the bundle and write the file; modify the bundle.generate invocation
to include inlineDynamicImports: true alongside format: 'esm'.
| try { | ||
| const { readFileSync } = await import('node:fs'); | ||
| prevBaseline = JSON.parse(readFileSync(prevBaselinePath, 'utf8')) as Baseline; | ||
| } catch { | ||
| // No baseline available — first run or local build | ||
| } |
There was a problem hiding this comment.
Only treat a missing baseline as first-run.
This catch also hides parse and permission errors, so a broken downloaded baseline silently turns the report into all 🆕 rows. Restrict the fallback to ENOENT and rethrow everything else.
Possible fix
-} catch {
- // No baseline available — first run or local build
+} catch (error) {
+ if ((error as NodeJS.ErrnoException).code !== 'ENOENT') {
+ throw error;
+ }
+ // No baseline available — first run or local build
}📝 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.
| try { | |
| const { readFileSync } = await import('node:fs'); | |
| prevBaseline = JSON.parse(readFileSync(prevBaselinePath, 'utf8')) as Baseline; | |
| } catch { | |
| // No baseline available — first run or local build | |
| } | |
| try { | |
| const { readFileSync } = await import('node:fs'); | |
| prevBaseline = JSON.parse(readFileSync(prevBaselinePath, 'utf8')) as Baseline; | |
| } catch (error) { | |
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| throw error; | |
| } | |
| // No baseline available — first run or local build | |
| } |
🤖 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/src/bundle.ts` around lines 122 - 127, The current
try/catch around reading and parsing prevBaselinePath swallows all errors
(including JSON parse and permission errors) causing broken baselines to be
treated as first-run; change the catch in the block that imports node:fs and
sets prevBaseline to only suppress missing-file errors by inspecting the caught
error (e.g., check err.code === 'ENOENT' or err.code === 'ENOTDIR') and rethrow
any other errors so parse/permission issues surface; update references to
prevBaseline and prevBaselinePath accordingly so only a true "file not found"
falls through to the "first run" behavior.
ryanbas21
force-pushed
the
bundle-size-app-checker
branch
from
1c62a75 to
1601090
Compare
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