Optimize @primer/octicons-react for codesplitting and tree-shaking - #1245
Conversation
🦋 Changeset detectedLatest commit: c6db8e1 The changes in this PR will be included in the next version bump. 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 |
There was a problem hiding this comment.
Pull request overview
This PR restructures @primer/octicons-react’s build output to enable per-icon subpath imports for code-splitting while keeping the existing barrel export tree-shakeable, and it removes per-icon runtime factory work by generating finished forwardRef components that share a renderOcticon runtime.
Changes:
- Added a shared
renderOcticonruntime and updatedcreateIconComponentto delegate rendering to it. - Updated the generator + Rollup build to emit one module per icon (plus a pure re-export barrel) and exposed per-icon subpath exports via
package.json. - Added tests/fixtures to validate codesplitting behavior and updated the tree-shaking snapshot; added a changeset for a minor release.
Show a summary per file
| File | Description |
|---|---|
| lib/octicons_react/src/renderOcticon.js | Introduces shared SVG render/runtime logic used by generated per-icon components. |
| lib/octicons_react/src/createIconComponent.js | Refactors the factory to delegate rendering to renderOcticon. |
| lib/octicons_react/script/build.js | Generates per-icon JS modules + barrel and per-icon .d.ts modules + barrel. |
| lib/octicons_react/script/types.js | Copies per-icon declaration files into dist/icons and writes dist/index.d.ts/index.d.mts. |
| lib/octicons_react/rollup.config.js | Switches to multi-entry ESM output so dist/ mirrors per-icon modules. |
| lib/octicons_react/package.json | Adds exports["./*"] for per-icon subpath imports; preserves . barrel. |
| lib/octicons_react/src/index.d.ts | Updates type barrel to point at the generated per-icon barrel. |
| lib/octicons_react/tests/tree-shaking.test.js | Updates snapshot to reflect new output size. |
| lib/octicons_react/tests/codesplitting.test.js | Adds regression tests for per-icon modules, pure barrel, exports, and code-splitting. |
| lib/octicons_react/tests/fixtures/dynamic-imports.mjs | Adds a fixture that triggers separate dynamic import chunks. |
| .changeset/octicons-react-codesplitting.md | Declares a minor release with the new codesplitting/tree-shaking behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/13 changed files
- Comments generated: 1
- Review effort level: Low
) * Initial plan * Remove stray npm lockfile and restore yarn.lock to match main - Delete lib/octicons_react/package-lock.json (added by accident when npm install was run in a Yarn repo) - Restore lib/octicons_react/yarn.lock to match main (no dependency changes in this PR, so yarn.lock churn was unintended side effect of npm install) yarn install --frozen-lockfile succeeds with the restored lockfile. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
joshblack
left a comment
There was a problem hiding this comment.
Looks great! Thanks for putting this up. Just had a couple of comments 👀
Co-authored-by: Matthew Costabile <mattcosta7@github.com>
joshblack
left a comment
There was a problem hiding this comment.
Let's try it out! @mattcosta7 did you want to test this out in github-ui first or do you feel like it's good to go in a release and we can go from there?
Description
@primer/octicons-reactshipped a single bundled ESM file built from one entry point. It tree-shook static named imports, but there was no per-icon module (so icons couldn't be dynamically imported/code-split), and every icon was constructed at module-eval time via acreateIconComponentfactory call.This splits output to one module per icon, pre-transforms icons into finished components, and adds a subpath export — all additive and backward compatible.
Output: one module per icon + pure re-export barrel
script/build.jsemitssrc/__generated__/icons/<Icon>.jsper icon plus a barrelicons/index.js.rollup.config.jsuses a multi-entry ESM build sodist/mirrors the tree:dist/index.esm.mjs(barrel),dist/icons/AlertIcon.mjs, and a sharedrenderOcticonchunk. UMD build retained.package.jsonexportsadds"./*"(per-icon.mjs+.d.ts) alongside the"."barrel.Pre-transformed components (no runtime factory)
renderOcticonruntime fromcreateIconComponent's render body;createIconComponentnow delegates to it./*#__PURE__*/forwardRefcomponents with statically-knownheights/svgDataByHeight— no factory work on import - this is a minor startup cost win, on the order of .1ms - since we do't need to evaluate a bunch of duplicative workTypes
.d.ts+ sharedtypes.d.tsgenerated and copied intodist/icons(script/types.js), so subpath imports resolve types.Consumers get both access patterns from the same package:
Generated per-icon module:
Bundle size (rollup, unminified):
import {AlertIcon}import *import *)Spritesheet/
<symbol>exploration is intentionally out of scope.