fix(concerto-core): remove ESM entrypoint from sideEffects to enable package-level tree shaking - #1263
Draft
mttrbrts wants to merge 2 commits into
Draft
fix(concerto-core): remove ESM entrypoint from sideEffects to enable package-level tree shaking#1263mttrbrts wants to merge 2 commits into
mttrbrts wants to merge 2 commits into
Conversation
Signed-off-by: Rishabh Jain <rishabhj2005@email.com>
…package-level tree shaking Listing `./dist/esm/index.mjs` in sideEffects prevented bundlers from skipping concerto-core entirely when nothing from it is imported. Instead, emit `dist/esm/dayjs-setup.js` as a separate file and reference that in sideEffects — keeping the real side effect (dayjs.extend() calls) declared while allowing the main ESM bundle to be tree-shaken at the package boundary. Verified: importing only concerto-util produces a 59 KB bundle; adding concerto-core jumps to 1.46 MB, confirming the package is correctly excluded when unused. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Matt Roberts <code@rbrts.uk>
5 tasks
Contributor
|
Hi @mttrbrts, I think I would prefer |
Contributor
|
This PR is stale because it has been open with no activity. Remove the stale label or comment to keep it active. Only items with maintainer engagement are auto-closed. |
Contributor
|
@github unstale |
Contributor
|
This PR is stale because it has been open with no activity. Remove the stale label or comment to keep it active. Only items with maintainer engagement are auto-closed. |
Contributor
|
@github unstale |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up fix to PR #1262 (tree-shakeable entrypoints), found during verification of tree-shaking behaviour. Intended to be merged into the
Rishabh060105/issue-1261-tree-shakingbranch, not directly into main.packages/concerto-core/package.json: removes./dist/esm/index.mjsfromsideEffects; adds./dist/esm/dayjs-setup.jsinsteadscripts/build-esm.js: adds a second esbuild pass that emitsdist/esm/dayjs-setup.jsas a separately-referenced file so thesideEffectsentry points to a real output fileWhy the original
sideEffectsentry was wrong:Listing
./dist/esm/index.mjsinsideEffectstells bundlers "this file has side effects — always include it in full." That prevents the package from being excluded when none of its exports are actually imported. The real side effect isdayjs.extend()indayjs-setup.ts, not the whole index. PointingsideEffectsat the separately-emitteddist/esm/dayjs-setup.jskeeps the side effect declared correctly while allowing the main bundle to be skipped at the package boundary.Verified before/after with Rollup:
concerto-util(notconcerto-core)ModelManagerfromconcerto-coreWithout this fix, even the "util only" bundle would include the full
concerto-coreESM file because it was marked as having side effects.Remaining known limitation (out of scope for this fix): per-export granular tree shaking within
concerto-coredoes not work because the ESM build is a pre-bundled CJS-in-ESM blob (__commonJS/__esmwrappers). Fixing that requires migrating source files fromimport X = require()/export = Xpatterns to native ESM — a larger source refactor for a future PR.Test plan
npm run build:esminpackages/concerto-coreproduces bothdist/esm/index.mjsanddist/esm/dayjs-setup.jsconcerto-utildoes not includeconcerto-coree2e/tests/browser-bundles.spec.ts)DateTimeUtil.setCurrentTime()still works (dayjs plugins correctly extended via inlined setup)Related Issues
🤖 Generated with Claude Code