Skip to content

refactor: icons chunk#4567

Draft
jog1t wants to merge 1 commit into03-25-refactor_frontend_actor_list_upliftfrom
04-06-refactor_icons_chunk
Draft

refactor: icons chunk#4567
jog1t wants to merge 1 commit into03-25-refactor_frontend_actor_list_upliftfrom
04-06-refactor_icons_chunk

Conversation

@jog1t
Copy link
Copy Markdown
Contributor

@jog1t jog1t commented Apr 6, 2026

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@railway-app
Copy link
Copy Markdown

railway-app bot commented Apr 6, 2026

🚅 Deployed to the rivet-pr-4567 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Apr 15, 2026 at 12:58 am
website 😴 Sleeping (View Logs) Web Apr 15, 2026 at 12:50 am
frontend-inspector 😴 Sleeping (View Logs) Web Apr 14, 2026 at 9:18 pm
mcp-hub ✅ Success (View Logs) Web Apr 14, 2026 at 7:46 pm
ladle ❌ Build Failed (View Logs) Web Apr 14, 2026 at 7:45 pm
kitchen-sink ❌ Build Failed (View Logs) Web Apr 14, 2026 at 7:45 pm

Copy link
Copy Markdown
Contributor Author

jog1t commented Apr 6, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jog1t jog1t mentioned this pull request Apr 6, 2026
11 tasks
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Apr 6, 2026

More templates

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@4567

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@4567

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@4567

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@4567

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@4567

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@4567

@rivetkit/sqlite-vfs

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sqlite-vfs@4567

@rivetkit/traces

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/traces@4567

@rivetkit/workflow-engine

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/workflow-engine@4567

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4567

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@4567

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@4567

commit: e46d451

@claude
Copy link
Copy Markdown

claude bot commented Apr 6, 2026

Code Review: refactor: icons chunk

Overview

This PR refactors the @rivet-gg/icons package from a single monolithic bundle (dist/index.gen.js, ~150K lines) into per-icon chunk files (dist/icons/faXxx.js) generated by esbuild code-splitting. This is the right direction: consumers can tree-shake at the individual icon level rather than pulling in the entire Font Awesome library.

The PR is in DRAFT status with an empty description.


Critical Issues

Legal banner is missing from all generated dist files.
The LEGAL_BANNER constant in vendor-icons.js is applied to src/index.gen.js and src/index.gen.ts, but the esbuild.build() call has no banner option. None of the 3,000+ dist/icons/*.js files or dist/icon-component.js carry the Font Awesome Pro legal notice. These files are committed to a public repository under a restrictive license. Fix: add banner: { js: LEGAL_BANNER } to the esbuild options and regenerate.


Non-Critical Issues

CJS interop boilerplate is repeated in every icon chunk.
Each of the ~3,000 icon files contains the same ~25-line __create / __defProp / __copyProps / __toESM / __commonJS helper block. This is inherent because Font Awesome Pro packages are CommonJS and esbuild inlines these helpers per chunk. There is no simple fix, but it is worth documenting as a known trade-off given the impact on committed repo size.

dist/icon-component.js lacks the generated-file header.
All other generated files include @ts-nocheck and a do-not-modify comment. The icon-component.js esbuild output has no such header, making it inconsistent and potentially editable by mistake.

Changes beyond 3,000 files are not visible in this review.
GitHub's API caps PR file listings at 3,000 files. Please confirm the following are correct in the full changeset:

  • dist/index.js was regenerated and correctly barrel-exports from the new chunk structure
  • scripts/vendor-icons.js reflects any entryPoints or output-directory changes that produce dist/icons/
  • src/index.gen.ts type declarations remain in sync with the new dist layout

PR description is empty.
The checklist is unchecked with no summary, motivation, or test plan. Noting the build command and before/after dist sizes would help reviewers understand the change.


Observations

  • The code-splitting approach (barrel dist/index.js + per-icon dist/icons/ chunks) is correct for downstream tree-shaking.
  • Externalizing free FA packages, react, and @fortawesome/react-fontawesome as peer dependencies is correct.
  • Custom and pro icon files (15 lines each) are correctly slimmer than free FA icons since they don't need the CJS interop wrapper.
  • Correction from the prior review: splitting: true is already present in the esbuild config -- it was not missing.

Suggested Actions Before Merging

  1. Add banner: { js: LEGAL_BANNER } to the esbuild config and regenerate all dist files.
  2. Fill in the PR description with the build command and a before/after size comparison.
  3. Confirm dist/index.js correctly re-exports from the new chunk layout.

@github-actions
Copy link
Copy Markdown
Contributor

Preview packages published to npm

Install with:

npm install rivetkit@pr-4567

All packages published as 0.0.0-pr.4567.065c590 with tag pr-4567.

Engine binary is shipped via @rivetkit/engine-cli on linux-x64-musl, linux-arm64-musl, darwin-x64, and darwin-arm64. Windows users should use the release installer or set RIVET_ENGINE_BINARY.

Docker images:

docker pull rivetdev/engine:slim-065c590
docker pull rivetdev/engine:full-065c590
Individual packages
npm install rivetkit@pr-4567
npm install @rivetkit/react@pr-4567
npm install @rivetkit/rivetkit-native@pr-4567
npm install @rivetkit/sqlite-wasm@pr-4567
npm install @rivetkit/workflow-engine@pr-4567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant