Treeshaking improvement #63
Conversation
✅ Deploy Preview for fluffy-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThe package exports structure is expanded to support granular entry points (components, ui, hooks, utils) with corresponding ES module and CommonJS variants. Build configuration is restructured to dynamically discover library entry points via glob patterns, generate separate ES and CommonJS outputs, and introduce new CSS handling and external dependency management. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
28-79: Reorder exports: place wildcard./*after specific exports.The export configuration assumes the correct file structure (hooks.ts and utils.ts as single files, components/ui as directories with index files), which is verified. However, the wildcard export
"./*"on line 28 should come after specific exports for clarity and best practices. Reorder to:"exports": { ".": { ... }, "./components": { ... }, "./ui": { ... }, "./hooks": { ... }, "./utils": { ... }, "./*": { ... }, "./style.css": "...", "./theme.css": "..." }This ensures specific exports are matched first, preventing any potential bundler ambiguity.
🧹 Nitpick comments (2)
vite.config.ts (1)
47-61: Consider removing redundant React externals.Lines 49-52 explicitly list React packages, but line 53 spreads
peerDependencieswhich already includesreactandreact-dom(lines 137-139 in package.json). While not harmful, this creates redundancy.Consider simplifying to:
external: [ - // React - ensure all React imports are external - "react", - "react-dom", "react/jsx-runtime", /^react($|\/)/, // Catches all react imports including subpaths ...Object.keys(peerDependencies),package.json (1)
11-12: Redundant sideEffects entry.Line 12 is redundant since the
"**/*.css"pattern on line 11 already matches"./dist/global.css".Consider simplifying:
"sideEffects": [ - "**/*.css", - "./dist/global.css" + "**/*.css" ],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.json(5 hunks)vite.config.ts(1 hunks)
🔇 Additional comments (7)
vite.config.ts (5)
2-2: LGTM: Necessary imports for dynamic entry collection.The imports support the new glob-based entry discovery mechanism.
Also applies to: 8-8
16-28: LGTM: Granular DTS configuration supports tree-shaking.The configuration correctly generates separate type definitions while excluding test and story files. Setting
rollupTypes: falseis appropriate for maintaining granular type imports.
62-76: LGTM: Output configuration enables tree-shaking.The dual output format with
preserveModules: trueis essential for tree-shaking. The naming pattern aligns correctly with package.json exports.
78-79: LGTM: CSS configuration appropriate for library distribution.Disabling CSS code splitting and minification is correct for a library, allowing consuming applications to handle optimization.
34-42: Ignore pattern syntax is valid—no changes needed.The glob ignore patterns use correct v11 syntax:
**for recursive traversal, brace expansion{ts,tsx}for alternatives, and/**to match directory contents. The patterns align with the dts plugin's exclude configuration. The key transformation correctly strips the "lib/" prefix and file extensions.package.json (2)
14-15: LGTM: Entry points align with Vite output.The updated
mainandmoduleentries correctly match the Rollup output configuration in vite.config.ts.
138-139: LGTM: tailwind-merge correctly added as peer dependency.Moving
tailwind-mergeto peer dependencies enables better tree-shaking by requiring consumers to provide their own instance. The version matches the devDependency entry and is correctly externalized in vite.config.ts.
package.json
Outdated
| "test:watch": "vitest", | ||
| "build": "storybook build", | ||
| "build:lib": "tsc && vite build && cp ./lib/theme.css ./dist/theme.css", | ||
| "build:lib": "rm -rf dist && vite build && cp ./lib/theme.css ./dist/theme.css && mv ./dist/shared-ui.css ./dist/style.css", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Vite CSS output filename assumption.
The build script assumes Vite outputs dist/shared-ui.css and renames it to dist/style.css. Confirm this filename matches Vite's actual output, as it typically derives from the library name or entry point. If the filename is incorrect, the mv command will fail.
🏁 Script executed:
#!/bin/bash
# Description: Check what CSS file Vite actually outputs during build
# Run a test build and check the CSS output
echo "=== Running build to check CSS output filename ==="
npm run build:lib 2>&1 | grep -i "css\|dist"
echo -e "\n=== Checking actual CSS files in dist ==="
fd -e css "" dist/ 2>/dev/null || echo "No dist directory found - build may have failed"Length of output: 252
🏁 Script executed:
#!/bin/bash
# Check vite configuration and package.json for CSS output details
echo "=== Checking for vite.config files ==="
fd -e ts -e js "" . -g "*vite.config*"
echo -e "\n=== Extracting library name and build config from package.json ==="
cat package.json | grep -A 5 '"name":'
echo -e "\n=== Looking for vite.config content ==="
fd -e ts -e js "" . -g "*vite.config*" -x cat {}
echo -e "\n=== Checking for build output or lib entry in package.json ==="
cat package.json | grep -E '"(main|lib|export|build)"'Length of output: 11902
---
🌐 Web query:
```web
Vite CSS output filename multi-entry glob preserveModules
💡 Result:
Short answer: control CSS filenames via rollupOptions.output.assetFileNames (or build.lib.cssFileName in lib mode). For multi-entry + preserveModules you must use an assetFileNames function and (because Rollup’s assetInfo lacks full source path) give each CSS a distinct source name (or rename imports) or emit assets yourself; otherwise multiple entry styles often become index..css and collide.
Example (glob entries + preserveModules):
- Use glob input, preserveModules, and customise assetFileNames to catch .css:
- assetFileNames receives assetInfo; test assetInfo.name or assetInfo.source and return a pattern like assets/css/[name]-[hash][extname].
vite.config.js example:
import { defineConfig } from 'vite'
import glob from 'glob'
import path from 'path'
export default defineConfig({
build: {
rollupOptions: {
input: glob.sync(path.resolve(__dirname, 'src/**/index.{js,ts}')),
output: {
preserveModules: true,
preserveModulesRoot: 'src',
entryFileNames: 'js/[name].js',
chunkFileNames: 'js/chunks/[name]-[hash].js',
assetFileNames: (assetInfo) => {
if (assetInfo.name?.endsWith('.css')) {
// assetInfo.name often is 'index.css' — to avoid collisions, prefer unique css import names
return 'css/[name]-[hash][extname]'
}
return 'assets/[name]-[hash][extname]'
}
}
}
}
})
Notes / caveats:
- Vite docs: build.rollupOptions.output maps to Rollup’s output options; assetFileNames supports functions and placeholders. [1][2]
- Vite lib mode has build.lib.cssFileName for single-name CSS outputs — not helpful for multi-entry preserveModules. [1]
- Because Rollup’s assetInfo often only exposes the emitted asset name (e.g., index.css) and not the original folder, identical source names can produce indistinguishable assetInfo.name values; recommended workarounds:
- Give each entry’s stylesheet a unique filename (e.g., home.style.css) so [name] reflects uniqueness. [3][4]
- Or programmatically emit CSS assets with distinct names from a plugin so assetFileNames can pick them reliably. See Vite/Rollup discussions for multi-entry preserveModules patterns. [4][5]
References:
- Vite build options (cssFileName / rollupOptions). [1]
- Rollup output.assetFileNames docs. [2]
- Community examples / StackOverflow on assetFileNames for CSS naming. [3]
- Vite discussion: folder-as-entry / preserveModules + glob examples. [4]
- Example answers showing assetFileNames usage for CSS. [5]
Fix build script CSS filename or configure Vite assetFileNames.
The build script assumes Vite outputs dist/shared-ui.css, but with multi-entry glob and preserveModules: true configuration, Vite generates CSS with generic names (e.g., index.css) or hashed filenames instead. The mv command will fail with file not found.
Either:
- Configure
assetFileNamesinvite.config.tsto explicitly outputshared-ui.css, or - Update the build script to use the actual CSS filename Vite generates
🤖 Prompt for AI Agents
In package.json around line 86, the build script assumes a fixed output filename
dist/shared-ui.css which can be incorrect with Vite's
multi-entry/preserveModules (Vite may emit index.css or hashed names); update
the build to either (A) set assetFileNames in vite.config.ts so the CSS is
emitted with a deterministic name like shared-ui.css, or (B) change the npm
build step to locate the generated CSS file at runtime (e.g., glob for *.css in
dist and rename/copy the found file to dist/style.css) so the mv step no longer
fails when Vite uses different names.
There was a problem hiding this comment.
good point, what about now ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of the build script and vite config
echo "=== Current build:lib script in package.json ==="
cat package.json | grep -A 1 '"build:lib"'
echo -e "\n=== Checking vite.config.ts for assetFileNames configuration ==="
cat vite.config.ts
echo -e "\n=== Checking if there's a .coderabbit.yaml or recent commits ==="
git log --oneline -5 --all 2>/dev/null || echo "Git log not available"
Length of output: 3140
@chmurson I can see the current configuration, but I don't see any changes that explicitly guarantee the CSS filename. While Vite might output shared-ui.css based on your package name with cssCodeSplit: false, this isn't explicitly configured and could change.
To make the build robust, I recommend adding assetFileNames configuration to vite.config.ts:
output: [
{
format: "es",
preserveModules: true,
preserveModulesRoot: "lib",
entryFileNames: "[name].es.js",
assetFileNames: (assetInfo) => {
if (assetInfo.name?.endsWith('.css')) {
return 'shared-ui.css';
}
return '[name][extname]';
},
},
{
format: "cjs",
preserveModules: true,
preserveModulesRoot: "lib",
entryFileNames: "[name].cjs.js",
exports: "named",
assetFileNames: (assetInfo) => {
if (assetInfo.name?.endsWith('.css')) {
return 'shared-ui.css';
}
return '[name][extname]';
},
},
],This explicitly tells Vite to output CSS as dist/shared-ui.css, ensuring your mv command will always work. Alternatively, if you've tested the build and confirmed it already outputs shared-ui.css, then you're good to go! 🎯
There was a problem hiding this comment.
oppsie daisy; I didn't push! what about now ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the latest vite.config.ts and package.json to check for changes
echo "=== Current vite.config.ts content ==="
cat vite.config.ts
echo -e "\n=== Current build:lib script from package.json ==="
cat package.json | grep -A 1 '"build:lib"'
echo -e "\n=== Checking for assetFileNames in vite.config.ts ==="
rg -n "assetFileNames" vite.config.ts || echo "No assetFileNames configuration found"
Length of output: 3018
@chmurson Great! I can see you've added assetFileNames: "styles.css" to both output configurations in vite.config.ts. However, there's a filename mismatch issue:
Problem:
- Vite will output:
dist/styles.css(plural - "styles") - But your
package.jsonexports:"./style.css": "./dist/style.css"(singular - "style")
This means consumers importing @fluffylabs/shared-ui/style.css will get a 404 error because the file will be named styles.css, not style.css.
Solution: Change the assetFileNames in both output configurations to use the singular form:
assetFileNames: "style.css", // Changed from "styles.css"This will match your package.json exports and ensure consumers can successfully import the CSS file. 🎯
There was a problem hiding this comment.
oookie! your comments are bulletproof. What about now?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of vite.config.ts and package.json after the fix
echo "=== Current vite.config.ts content ==="
cat vite.config.ts
echo -e "\n=== Checking assetFileNames in vite.config.ts ==="
rg -n "assetFileNames" vite.config.ts || echo "No assetFileNames found"
echo -e "\n=== Current build:lib script from package.json ==="
cat package.json | grep -A 1 '"build:lib"'
echo -e "\n=== CSS exports from package.json ==="
cat package.json | grep -A 1 'style.css'
Length of output: 3130
@chmurson Almost there! 🎯 You removed the mv command from the build script (good!), but the filename mismatch is still present:
Current issue:
vite.config.ts(lines 68 & 76):assetFileNames: "styles.css"❌ (plural)package.jsonexports:"./style.css": "./dist/style.css"✅ (singular)
Fix needed:
Change both occurrences in vite.config.ts from "styles.css" to "style.css":
output: [
{
format: "es",
preserveModules: true,
preserveModulesRoot: "lib",
entryFileNames: "[name].es.js",
assetFileNames: "style.css", // Change this line
},
{
format: "cjs",
preserveModules: true,
preserveModulesRoot: "lib",
entryFileNames: "[name].cjs.js",
exports: "named",
assetFileNames: "style.css", // Change this line
},
]Once this is done, Vite will output dist/style.css, matching your package.json exports perfectly! 🚀
There was a problem hiding this comment.
oh man! i didn't push again, what about now ?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current state of vite.config.ts after the latest push
echo "=== Current vite.config.ts content ==="
cat vite.config.ts
echo -e "\n=== Checking assetFileNames configuration ==="
rg -n "assetFileNames" vite.config.ts || echo "No assetFileNames found"
echo -e "\n=== CSS exports from package.json for comparison ==="
cat package.json | grep -B 1 -A 1 '"./style.css"'
Length of output: 2918
@chmurson Perfect! 🎉 Now everything is aligned:
- ✅
vite.config.tsoutputs:dist/style.css(lines 68 & 76) - ✅
package.jsonexports:./style.css→./dist/style.css - ✅ Build script no longer needs the
mvcommand
The configuration is now bulletproof! Vite will consistently output dist/style.css, and consumers can reliably import it via @fluffylabs/shared-ui/style.css. Your treeshaking improvements should work perfectly now! 🚀
/)/)
(^.^)
(> <) All fixed!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
package.json
Outdated
| "@testing-library/jest-dom": "^6.6.3", | ||
| "@testing-library/react": "^16.2.0", | ||
| "@testing-library/user-event": "^14.6.1", | ||
| "@types/glob": "^8.1.0", |
There was a problem hiding this comment.
🧩 Analysis chain
Verify @types/glob version compatibility.
There's a major version mismatch: [email protected] but @types/[email protected]. Confirm that the type definitions are compatible, as glob v11 may have breaking API changes not reflected in v8 types.
Also applies to: 120-120
🌐 Web query:
Are @types/glob version 8 type definitions compatible with glob version 11?
💡 Result:
Short answer: No — glob v11 ships its own TypeScript types, so you should not rely on @types/glob v8 (which targets older releases) — remove the @types package and use the types bundled with glob 11 to avoid conflicts. [1][2]
Sources:
- @types/glob package info (DefinitelyTyped/@types versions). [1]
- glob v11 package (shows bundled TypeScript interfaces/types). [2]
Remove @types/glob — glob v11 ships bundled TypeScript types.
glob v11 ships its own TypeScript types, so @types/glob v8 should not be used to avoid conflicts. Remove @types/glob@^8.1.0 from devDependencies (line 104).
Also applies to: 120-120
🤖 Prompt for AI Agents
In package.json around line 104 (and also check line 120), remove the
devDependency entry for "@types/glob": "^8.1.0" because glob v11 includes its
own types; update the devDependencies block to delete that line, save the file,
and run the package manager (npm/yarn/pnpm) to update lockfile (or regenerate
it) so the @types/glob package is no longer installed.
There was a problem hiding this comment.
sounds good ! removed
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if @types/glob is still present in package.json
echo "=== Checking for @types/glob in package.json ==="
cat package.json | jq '.devDependencies["@types/glob"]'
echo ""
echo "=== Checking glob version ==="
cat package.json | jq '.devDependencies.glob'
echo ""
echo "=== Full devDependencies with grep for glob-related packages ==="
cat package.json | jq '.devDependencies' | grep -i glob
Length of output: 181
@chmurson Perfect! 🎉 The fix looks great. I can confirm that:
✅ @types/glob has been successfully removed from devDependencies
✅ glob@^11.0.3 remains (with its bundled TypeScript types)
You're all set — no more type definition conflicts. The library will now use the built-in types that ship with glob v11.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
thank you for using us ! :D
Example Graypaper:
before
dist/assets/index-DhoV5Ql0.js 1,728.19 kB │ gzip: 537.61 kBafter
dist/assets/index-VMDfcZXz.js 1,633.25 kB │ gzip: 510.95 kBbefore
after