Clarify and Fix Plugin CSS Bundling #20
Conversation
WalkthroughThe changes modify import syntax and CSS file reference in a TypeScript widget index file, broaden the webpack entry glob pattern to include nested subdirectories, refactor entry object construction and formatting in the webpack config, and remove the README copy step from the build script in package.json. No functional logic or exported interfaces were altered. Changes
Estimated code review effort1 (~5 minutes) Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
webpack.config.js (1)
22-26: Potential key-collision in the dynamicentrymap
path.parse(el).nameextracts only the basename of every matched file.
With the new recursive glob (./src/widgets/**/*.tsx) two widgets in different sub-folders that share the same filename (e.g.index.tsx) will overwrite each other in theobjliteral, silently dropping one bundle.Consider deriving the key from the relative path to guarantee uniqueness:
-entry: glob.sync('./src/widgets/**/*.tsx').reduce((obj, el) => { - obj[path.parse(el).name] = el; - obj[path.parse(el).name + SANDBOX_SUFFIX] = el; +entry: glob.sync('./src/widgets/**/*.tsx').reduce((obj, el) => { + const rel = path + .relative('src/widgets', el) // e.g. foo/bar/index.tsx + .replace(/\.[tj]sx?$/, '') // strip extension + .replace(/\\/g, '/'); // Windows → POSIX + + obj[rel] = el; + obj[`${rel}${SANDBOX_SUFFIX}`] = el; return obj; }, {}),
🧹 Nitpick comments (2)
webpack.config.js (1)
50-54: Minor: avoid pushingundefinedinto theusearrayWhen
isDevelopmentisfalsethe current ternary yields the loader instance; whentruethe expression yields'style-loader', so both paths are OK.
However, an earlier pattern (isDevelopment ? undefined : …) addedundefinedintopluginsand was later filtered via.filter(Boolean).
For consistency you could follow the same pattern here or vice-versa.
No functional impact, purely cosmetic.src/widgets/index.tsx (1)
2-4: Duplicate / unclear CSS importsBoth
../style.cssand../index.cssare imported.
Given the PR’s intent to standardise onindex.css, confirm thatstyle.cssis still required; otherwise drop it to avoid duplicate rules and larger bundles.-import '../style.css'; -import '../index.css'; +import '../index.css';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/widgets/index.tsx(1 hunks)webpack.config.js(4 hunks)
🔇 Additional comments (2)
webpack.config.js (1)
91-93: 👍 Copy-plugin simplification looks goodRemoving the redundant
README.mdcopy makes the bundle leaner and avoids leaking repo docs into user downloads.src/widgets/index.tsx (1)
1-1: Good use oftype-only importSwitching to
type ReactRNPluginprevents tree-shaking from pulling runtime code into the bundle.
Nice touch.
… README.md to webpack copy patterns
I pushed some fixes, please review them. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll review the recent changes you've pushed. Let me examine the latest commits and file changes. 🏁 Script executed: Length of output: 6349 @coldenate, I've reviewed the current state of your files, and I notice some discrepancies between the intended changes described in the PR and the actual current state of the code. Here are the issues I found: Issues Found:1. Webpack Entry Pattern Not UpdatedThe entry: glob.sync('./src/widgets/**.tsx').reduce((obj, el) => {It should be updated to: entry: glob.sync('./src/widgets/**/*.tsx').reduce((obj, el) => {2. README.md Still Being CopiedThe new CopyPlugin({
patterns: [
{ from: 'public', to: '' },
{ from: 'README.md', to: '' }, // This should be removed
],
}),3. Duplicate CSS ImportsThe import '../style.css';
import '../index.css';Based on the PR objectives, it should only import 4. Missing Build Script UpdateThe Recommendations:
These fixes will align your code with the PR objectives of clarifying CSS bundling and removing unnecessary README copying. |
Problem
I found there was confusion with new plugin developers trying to use CSS styles in their Plugins. They would use
App.css, yetApp.csswould not ever be bundled properly. This led to situations where plugin-specific styles were not included in the final build, or all styles were merged into a single CSS file, making it unclear how to scope or reference styles per plugin.Solution
./src/widgets/**/*.tsxfor robust, recursive matching of plugin entry points.README.mdinto the build output, as it is not needed for end-users..tsxfile (plugin entry) is allowed one corresponding.cssfile, as per thewebpack.config.jssetup.index.cssinstead ofApp.css), and updated imports accordingly.index.tsx) to import../index.cssinstead of../App.css, aligning with the new convention and ensuring the correct CSS is bundled.Summary by CodeRabbit