scripts/treeshaking.ts (2)

32-63: Minor: bundle.close() skipped if generate() throws.

If bundle.generate() on Line 58 throws, bundle.close() on Line 59 is never called. Consider wrapping the bundle lifecycle in its own try/finally to ensure cleanup.

♻️ Proposed fix

   try {
     const bundle = await rollup({
       input: tmpFile,
       external: getExternals(),
       treeshake: {
         moduleSideEffects: false,
       },
       logLevel: 'silent',
       plugins: [
         {
           name: 'resolve-directory-index',
           resolveId(moduleId: string, importer: string | undefined) {
             if (importer != null && moduleId.startsWith('.')) {
               const resolved = path.resolve(path.dirname(importer), moduleId);
               if (fs.existsSync(resolved) && fs.statSync(resolved).isDirectory()) {
                 const indexFile = path.join(resolved, 'index.js');
                 if (fs.existsSync(indexFile)) {
                   return indexFile;
                 }
               }
             }
             return null;
           },
         },
       ],
     });
-    const output = await bundle.generate({ format: 'esm' });
-    await bundle.close();
-    return output.output;
+    try {
+      const output = await bundle.generate({ format: 'esm' });
+      return output.output;
+    } finally {
+      await bundle.close();
+    }
   } finally {
     fs.unlinkSync(tmpFile);
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/treeshaking.ts` around lines 32 - 63, The rollup bundle is not
guaranteed to be closed if bundle.generate() throws; wrap the rollup lifecycle
so bundle.close() always runs: after creating bundle = await rollup(...) keep
the outer finally that unlinks tmpFile, but introduce an inner try/finally
around await bundle.generate({ format: 'esm' }) that always calls await
bundle.close() in the finally block (only call close if bundle is defined),
ensuring bundle.generate, bundle.close, and rollup are referenced correctly and
tmpFile cleanup remains in the existing outer finally.

---

9-57: Significant code duplication with scripts/upload-bundle-analysis.js.

The getExternals() function, es6Entry and packageRoot constants, temp file naming approach, rollup configuration, and the resolve-directory-index plugin are duplicated across both files. The implementations are functionally identical, though getExternals() has minor stylistic differences (treeshaking.ts uses a specialDeps constant while the upload script inlines the dependency). If either copy is updated independently, the other will silently drift.

Consider extracting these utilities into a shared .mjs module that both scripts can import, or convert the upload script to TypeScript to share the module directly. This prevents maintenance burden and ensures consistency.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/treeshaking.ts` around lines 9 - 57, Duplicate utilities
(getExternals, es6Entry, packageRoot, tmp file naming, rollup config and the
resolve-directory-index plugin) are repeated between scripts/treeshaking.ts and
scripts/upload-bundle-analysis.js; extract them into a single shared module and
import it from both scripts to avoid drift. Create a shared module that exports
getExternals, es6Entry, packageRoot, a helper for creating tmp filenames, and a
function or object for the common rollup plugin/configuration, then replace the
local definitions in treeshaking.ts (symbols: getExternals, es6Entry,
packageRoot, tmpFile creation, and the inline plugin named
"resolve-directory-index") and in upload-bundle-analysis.js to import and use
the shared exports so both scripts use the exact same implementation.

scripts/treeshaking.test.ts (3)

50-98: Bundle size tests are functional; minor DRY opportunity.

The six bundle-size tests follow an identical pattern (treeshake → getBundleSize → assert > 0 → log). This is readable as-is, but if the list of bundles grows, consider a table-driven approach:

♻️ Optional: parameterize bundle size tests

const bundleSizeCases: [string, string | string[]][] = [
  ['Cartesian components', [...CARTESIAN_COMPONENT_NAMES]],
  ['Polar components', [...POLAR_COMPONENT_NAMES]],
  ['Treemap', 'Treemap'],
  ['SunburstChart', 'SunburstChart'],
  ['Sankey', 'Sankey'],
  ['Chart components', [...CHART_COMPONENT_NAMES]],
];

for (const [label, components] of bundleSizeCases) {
  it(`${label} bundle size`, async () => {
    const output = await treeshake(components);
    const size = getBundleSize(output);
    expect(size).toBeGreaterThan(0);
    console.log(`${label} bundle size: ${size} bytes`);
  }, 60_000);
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/treeshaking.test.ts` around lines 50 - 98, The tests repeat the
pattern treeshake → getBundleSize → expect(...) → console.log, so refactor to a
table-driven loop to remove duplication: create an array (e.g., bundleSizeCases)
of tuples pairing the human label with the components (use
CARTESIAN_COMPONENT_NAMES, POLAR_COMPONENT_NAMES, CHART_COMPONENT_NAMES or
string names like 'Treemap'), then iterate over that array to call
treeshake(components), compute getBundleSize(output), assert
expect(size).toBeGreaterThan(0) and log the label; keep the individual timeout
(60_000) for each generated it block and preserve test names by using the label
when creating the it(...) description.

---

18-20: knownExpectedBundle is mapped but never used inside the test body.

The expectedInBundle data from each known issue is stored in knownIssuesByComponent and read into knownExpectedBundle (line 24), but the test function (lines 26-34) never references it — it checks for all other tracked components, not specifically the expected ones.

This is fine for the it.fails() gating logic (you only need to know whether a known issue exists, not which components it documents). But then you could simplify to just checking for the component name's presence rather than storing the full expectedInBundle value:

♻️ Optional simplification

-const knownIssuesByComponent = Object.fromEntries(
-  knownTreeshakingIssues.map(issue => [issue.component, issue.expectedInBundle]),
-);
+const knownIssueComponents = new Set(
+  knownTreeshakingIssues.map(issue => issue.component),
+);

Then on line 24/36:

-    const knownExpectedBundle = knownIssuesByComponent[componentName];
     ...
-    if (knownExpectedBundle != null) {
+    if (knownIssueComponents.has(componentName)) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/treeshaking.test.ts` around lines 18 - 20, The test builds
knownIssuesByComponent from knownTreeshakingIssues but then reads
knownExpectedBundle (from knownIssuesByComponent) and never uses it in the test
body; either use the expectedInBundle value in the assertions inside the test
function (reference knownExpectedBundle and assert the bundle content matches
expectedInBundle for that component) or simplify the setup to only track
component names (e.g., a Set of component strings) and change the test to check
presence by component name instead of storing expectedInBundle; update either
the mapping creation (knownIssuesByComponent / knownTreeshakingIssues) and the
test assertion logic accordingly so the stored data is actually used or removed.

---

22-48: Well-designed dynamic test generation with it.fails() for known issues.

The pattern of using it.fails() to document known tree-shaking problems while keeping CI green — and having it break when the issue is fixed (prompting removal of the known-issue entry) — is a clean approach.

One minor observation: the test verifies that importing component X doesn't include other tracked components, but it doesn't verify that component X itself is present in the bundle. If tree-shaking were overly aggressive and removed the target component entirely, this test would still pass. Consider adding a positive assertion:

💡 Optional: verify the target component is present

     const testFn = async () => {
       const output = await treeshake(componentName);
+      const selfInBundle = findComponentsInBundle(output, [componentName]);
+      expect(selfInBundle, `${componentName} should be present in its own bundle`).toEqual([componentName]);
       const otherComponents = ALL_TRACKED_COMPONENT_NAMES.filter(n => n !== componentName);
       const unexpectedComponents = findComponentsInBundle(output, otherComponents);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/treeshaking.test.ts` around lines 22 - 48, The test currently only
asserts that importing componentName does NOT pull in other tracked components;
add a positive assertion that the target component is actually present in the
bundle to catch over-aggressive tree-shaking: after calling
treeshake(componentName) (in testFn) use findComponentsInBundle(output,
[componentName]) and assert that the returned list includes componentName (or is
non-empty) before asserting unexpectedComponents is empty; keep this check in
the same testFn so both the presence of the target and absence of others are
validated for functions like treeshake, findComponentsInBundle, and the loop
over ALL_TRACKED_COMPONENT_NAMES (including handling knownExpectedBundle /
it.fails).

scripts/upload-bundle-analysis.js (3)

119-140: Consider converting the .then() chain to async/await for readability.

The sequential chain of eight .then() calls is verbose. An async IIFE or top-level async function would be cleaner and easier to extend:

♻️ Optional refactor

(async () => {
  await uploadBundleAnalysis('lib', 'recharts/bundle-cjs');
  await uploadBundleAnalysis('es6', 'recharts/bundle-es6');
  await uploadBundleAnalysis('umd', 'recharts/bundle-umd');
  await generateAndUploadTreeshakingBundle(CARTESIAN_COMPONENTS, 'recharts/bundle-treeshaking-cartesian');
  await generateAndUploadTreeshakingBundle(POLAR_COMPONENTS, 'recharts/bundle-treeshaking-polar');
  await generateAndUploadTreeshakingBundle(['Treemap'], 'recharts/bundle-treeshaking-treemap');
  await generateAndUploadTreeshakingBundle(['SunburstChart'], 'recharts/bundle-treeshaking-sunburst');
  await generateAndUploadTreeshakingBundle(['Sankey'], 'recharts/bundle-treeshaking-sankey');
})().catch(error => {
  console.error('Bundle analysis failed:', error);
  process.exitCode = 1;
});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/upload-bundle-analysis.js` around lines 119 - 140, Replace the long
Promise chain with an async flow: wrap the sequential calls to
uploadBundleAnalysis and generateAndUploadTreeshakingBundle (references:
uploadBundleAnalysis, generateAndUploadTreeshakingBundle, CARTESIAN_COMPONENTS,
POLAR_COMPONENTS) in an async function or async IIFE and await each call in
order, and add a single catch at the end to log errors and set a non-zero
process.exitCode; this simplifies control flow and preserves sequential
execution while making it easier to extend.

---

39-86: Significant code duplication with scripts/treeshaking.ts.

generateTreeshakingBundle duplicates the rollup config (resolve-directory-index plugin, treeshake options, temp file pattern) from treeshake() in treeshaking.ts. Additionally, CARTESIAN_COMPONENTS and POLAR_COMPONENTS duplicate CARTESIAN_COMPONENT_NAMES and POLAR_COMPONENT_NAMES.

This creates a maintenance risk: changes to component lists, externals, or rollup config in one file but not the other will produce silently inconsistent results between CI bundle reports and test assertions.

If cross-language import is impractical, consider at minimum extracting the component name arrays to a shared JSON or .js file that both can consume.

Also applies to: 88-107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/upload-bundle-analysis.js` around lines 39 - 86, The
generateTreeshakingBundle function duplicates rollup config, the custom
resolve-directory-index plugin, temp-entry filename generation, and component
lists (CARTESIAN_COMPONENTS, POLAR_COMPONENTS) already defined alongside
treeshake() and CARTESIAN_COMPONENT_NAMES/POLAR_COMPONENT_NAMES in
scripts/treeshaking.ts; extract the shared pieces into a single small module
(e.g., export the component name arrays, getExternals(), a helper to create the
temp entry filename, and the resolve-directory-index plugin or a function that
returns the plugins array) and have both generateTreeshakingBundle and
treeshake() import and reuse those exports so there is one source of truth for
component lists, externals, and rollup configuration to avoid drift.

---

10-15: getExternals() is duplicated from scripts/treeshaking.ts.

This function is nearly identical to the one in treeshaking.ts. If the externals list diverges between the two files, bundle analysis results will be inconsistent with what the tests measure.

Since this file is .js and treeshaking.ts is TypeScript, direct import may not be straightforward. Consider extracting a shared .js module (or compiling the TS utilities) so both files reference a single source of truth for externals.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/upload-bundle-analysis.js` around lines 10 - 15, Duplicate
getExternals() logic should be centralized: extract the function from
scripts/upload-bundle-analysis.js into a shared JS module (e.g.,
scripts/getExternals.js) that exports getExternals, then replace the inline
implementation in upload-bundle-analysis.js and the one in treeshaking.ts to
import/require that shared module (or compile treeshaking.ts utilities to JS and
import them) so both use the same single source of truth; update references to
the unique symbol getExternals (and preserve behavior: reading package.json,
merging dependencies and peerDependencies, and the vendor regex entries) and run
tests to ensure bundle analysis and treeshaking use identical externals.