feat: added css to packem add command #53
Conversation
|
Warning Rate limit exceeded@prisis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 7 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces various changes across multiple files, primarily focusing on enhancing the configuration and functionality of the Packem build system. Key modifications include updates to the GitHub Actions workflow for job management, enhancements to CSS loader and minifier configurations, and improvements in the testing framework for the Packem CLI. The documentation has been refined for clarity, and new utility modules have been added to manage CSS dependencies effectively. Overall, the changes aim to improve the user experience and maintainability of the Packem project. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
Thank you for following the naming conventions! 🙏 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (27)
packages/packem/src/commands/utils/css-loader-dependencies.ts (2)
1-18: Add TypeScript types and documentation.Consider adding TypeScript types and JSDoc documentation to improve maintainability and developer experience.
+/** + * Maps CSS preprocessors to their required dependencies. + * Used by the packem add command to install necessary packages for CSS processing. + */ +type CssLoaderDependencyMap = { + [key: string]: string[]; +}; + -const cssLoaderDependencies = { +const cssLoaderDependencies: CssLoaderDependencyMap = { less: ["less"], lightningcss: ["lightningcss"], "node-sass": ["node-sass"],
5-14: Consider grouping PostCSS dependencies logically.The PostCSS dependencies are correctly listed but could be organized better by grouping related modules (e.g., all CSS modules-related packages together).
postcss: [ "postcss", "postcss-load-config", + // CSS Modules related "postcss-modules-extract-imports", "postcss-modules-local-by-default", "postcss-modules-scope", "postcss-modules-values", + // Utilities "postcss-value-parser", "icss-utils", ],examples/styles/packem.config.ts (2)
15-15: Consider optimizing loader order for better processing.While the current order works, consider arranging preprocessors (Less, Sass, Stylus) before PostCSS for a more typical processing pipeline: preprocessors → PostCSS → sourcemaps.
- loaders: [postcssLoader, stylusLoader, lessLoader, sassLoader, sourceMapLoader,], + loaders: [lessLoader, sassLoader, stylusLoader, postcssLoader, sourceMapLoader,],
16-16: Consider adding cssnano configuration options.While the default cssnano configuration is good, you might want to customize it based on your needs (e.g., autoprefixer options, custom preservation rules).
- minifier: cssnanoMinifier, + minifier: cssnanoMinifier({ + preset: ['default', { + discardComments: { removeAll: true }, + normalizeWhitespace: false + }] + }),examples/styles/package.json (2)
21-21: LGTM with a minor suggestion about version consistency.The new dependencies are up-to-date and appropriate for CSS processing capabilities. However, consider standardizing the version constraint style:
cssnanouses fixed version7.0.6icss-utilsandstylususe caret ranges^5.1.0and^0.64.0Consider using consistent version constraints across dependencies for easier maintenance:
- "cssnano": "7.0.6", + "cssnano": "^7.0.6",Also applies to: 23-23, 35-35
Line range hint
21-36: Consider documenting the purpose of multiple CSS preprocessors.The package includes three CSS preprocessors (Less, Sass, and Stylus). While this is appropriate for an examples project, it would be helpful to document why all three are included and how they demonstrate different use cases.
Consider adding a comment in the package.json or a README file explaining that this example demonstrates various CSS preprocessing options available in Packem.
packages/packem/src/rollup/watch.ts (2)
Line range hint
126-129: Improve comment style for better maintainability.Consider using a more descriptive comment that explains why this is necessary, rather than labeling it as a hack.
- // This is a hack to prevent caching when using isolated declarations or css loaders + // Disable caching for isolated declarations and CSS loaders to ensure fresh builds + // as these features require clean state for correct processing
Based on the gathered information, I can now provide the final response:
Watch mode test coverage needs improvement
The codebase lacks dedicated test coverage for watch mode functionality, particularly for the scenarios mentioned in the review comment. While there is a comprehensive watch implementation in
packages/packem/src/rollup/watch.ts, there are no corresponding test files that verify:
- Watch mode with CSS loaders
- Watch mode with isolated declarations
- Watch mode with both bundle and type declaration generation
The implementation handles these scenarios, but without proper test coverage, we cannot ensure their reliability across changes.
🔗 Analysis chain
Line range hint
126-129: Verify test coverage for cache handling scenarios.Please ensure that the following scenarios are covered in tests:
- Watch mode with CSS loaders
- Watch mode with isolated declarations
- Watch mode with both bundle and type declaration generation
Also applies to: 181-181, 207-207
Let me gather more information about watch mode test coverage specifically.
Let me try a different approach to find watch-related tests.
Let me check the content of the watch implementation file and related test files.
Let me check the integration tests for watch mode functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for relevant test files echo "Searching for test files covering watch functionality..." rg -l "watch" --glob "**/*.test.ts" --glob "**/*.spec.ts" # Look for specific test cases echo -e "\nSearching for cache-related test cases..." rg "useCache|isolatedDeclarations|css" --glob "**/*.test.ts" --glob "**/*.spec.ts"Length of output: 13940
Script:
#!/bin/bash # Search for watch mode related test cases echo "Searching for watch mode test cases..." rg "watch" -B 2 -A 2 --glob "**/*.test.ts" --glob "**/*.spec.ts" # Look for rollup watch configuration tests echo -e "\nSearching for rollup watch configuration tests..." rg "rollup.*watch" -B 2 -A 2 --glob "**/*.test.ts" --glob "**/*.spec.ts" # Check for any watch-related test utilities or helpers echo -e "\nSearching for watch test utilities..." rg "watch" --glob "**/test-utils.ts" --glob "**/test-helpers.ts"Length of output: 646
Script:
#!/bin/bash # Search for watch-related files and their content echo "Searching for watch-related files..." fd -e ts -e js watch # Look for watch configuration in test files echo -e "\nSearching for watch configuration in test files..." ast-grep --pattern 'watch: $_' # Check for rollup configuration tests echo -e "\nSearching for rollup configuration tests..." fd -e test.ts -e spec.ts rollupLength of output: 428
Script:
#!/bin/bash # Check watch implementation echo "Content of watch implementation:" cat packages/packem/src/rollup/watch.ts # Search for test files in the rollup directory echo -e "\nLooking for rollup test files:" fd -e test.ts -e spec.ts . packages/packem/__tests__/unit/rollup/ # Check content of any rollup config tests echo -e "\nContent of rollup config tests:" for f in $(fd -e test.ts -e spec.ts . packages/packem/__tests__/unit/rollup/); do echo "=== $f ===" cat "$f" doneLength of output: 19918
Script:
#!/bin/bash # Search for watch-related integration tests echo "Searching for watch mode integration tests..." rg -B 3 -A 3 "watch" packages/packem/__tests__/intigration/ # Check for watch mode configuration in integration tests echo -e "\nSearching for watch configuration in integration tests..." ast-grep --pattern 'watch: $_' packages/packem/__tests__/intigration/ # Look for test setup files that might contain watch mode setup echo -e "\nSearching for test setup files..." fd setup packages/packem/__tests__/Length of output: 484
.github/workflows/test.yml (1)
Line range hint
102-104: Fix logical error in node-sass removal condition.The current condition has a logical error that causes it to always evaluate to true. The version check should use AND operators instead of OR.
Apply this fix:
- if: "matrix.os == 'macos-latest' || matrix.node_version != '18' || matrix.node_version != '20'" + if: "matrix.os == 'macos-latest' || (matrix.node_version != '18' && matrix.node_version != '20')"This ensures node-sass is only removed for:
- macOS systems OR
- Node.js versions other than 18 and 20
packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (3)
Line range hint
46-251: Consider refactoring the process method for better maintainability.The
processmethod is quite complex with multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Configuration processing
- Plugin management
- Module handling
- Source map processing
- Output generation
Here's a suggested approach:
interface ProcessContext { config: PostCSSConfig; plugins: AcceptedPlugin[]; modulesExports: Record<string, string>; icssDependencies: string[]; // ... other shared state } class PostCSSLoader implements Loader<NonNullable<InternalStyleOptions["postcss"]>> { private async processConfig(): Promise<ProcessContext> { // Configuration loading logic } private setupPlugins(context: ProcessContext): void { // Plugin setup logic } private handleModules(context: ProcessContext): void { // Module processing logic } private processMessages(result: PostCSSResult, context: ProcessContext): void { // Message handling logic } private generateOutput(result: PostCSSResult, context: ProcessContext): string { // Output generation logic } async process({ code, extracted, map }): Promise<ProcessResult> { const context = await this.processConfig(); this.setupPlugins(context); // ... orchestrate the processing steps } }
Line range hint
28-28: Consider improving injector caching mechanism.The module-level
injectorIdvariable could lead to memory leaks in long-running processes. Consider moving this to an instance property or implementing a proper caching mechanism.Consider using a WeakMap or implementing a cache with proper cleanup:
class PostCSSLoader { private static injectorCache = new WeakMap<string, string>(); private getInjectorId(): string { const cacheKey = this.cwd as string; let injectorId = PostCSSLoader.injectorCache.get(cacheKey); if (!injectorId) { injectorId = resolve(["./runtime/inject-css"], { baseDirs: [join(baseDirectory, "..", "..")] }); PostCSSLoader.injectorCache.set(cacheKey, injectorId); } return `"${normalize(injectorId)}"`; } }Also applies to: 252-252
Line range hint
46-251: Enhance error handling and validation.The error handling could be more robust, particularly around configuration validation and plugin initialization.
Consider adding:
- Input validation
- Better error messages
- Error recovery strategies
interface ValidationResult { isValid: boolean; errors: string[]; } function validateConfig(config: unknown): ValidationResult { const errors: string[] = []; // Validation logic return { isValid: errors.length === 0, errors }; } class PostCSSLoader { private validateAndLoadConfig(): PostCSSConfig { const config = loadConfig(/*...*/); const validation = validateConfig(config); if (!validation.isValid) { throw new Error( `Invalid PostCSS configuration:\n${validation.errors.join('\n')}` ); } return config; } }packages/packem/__tests__/intigration/cli.test.ts (2)
Line range hint
1-400: Comprehensive test coverage with room for enhancement.The test suite thoroughly covers various CLI scenarios including:
- TypeScript configuration overrides
- Development/Production modes
- Directory cleaning options
- DTS generation
onSuccesscommand executionConsider adding the following test cases to improve coverage:
- Error handling for invalid
onSuccesscommands- Multiple
onSuccesscommands- Command execution in different environments (Windows/Unix)
Would you like me to help generate these additional test cases?
Fix typo in directory name: "intigration" should be "integration"
The directory
packages/packem/__tests__/intigration/exists with the misspelled name. This should be renamed to maintain code quality and professionalism.
- Location:
packages/packem/__tests__/intigration/🔗 Analysis chain
Line range hint
1-1: Fix typo in directory name: "intigration" should be "integration".The directory name contains a spelling error. Consider renaming the directory from "intigration" to "integration" to maintain professional quality.
Let's verify if this is the only occurrence of this typo:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of the misspelling rg -i "intigration"Length of output: 19
Script:
#!/bin/bash # Let's try a different approach using fd to find files and directories with this pattern fd -i "intigration" # Also check for any references in package.json, jest config, or similar files find . -type f -name "package.json" -o -name "jest.config.*" | xargs catLength of output: 49562
packages/packem/README.md (1)
Line range hint
519-903: Consider enhancing the CSS documentation with additional details.The CSS documentation is comprehensive, but consider adding the following information to make it even more helpful:
- Production optimization strategies for CSS (minification, autoprefixer, etc.)
- CSS Modules naming conventions and configuration options
- Examples of using CSS Modules with TypeScript (
.module.css.d.tsgeneration)packages/packem/src/commands/add.ts (6)
75-75: UserootDirectoryinstead ofprocess.cwd()for consistencyConsider using
rootDirectoryas the current working directory when installing packages to ensure consistency, especially ifoptions.diris specified.Apply this diff:
- await installPackage(typedocPackages, { cwd: process.cwd(), dev: true, silent: true }); + await installPackage(typedocPackages, { cwd: rootDirectory, dev: true, silent: true });
204-204: UserootDirectoryinstead ofprocess.cwd()for consistencySame as above.
Apply this diff:
- await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true }); + await installPackage(packagesToInstall, { cwd: rootDirectory, dev: true, silent: true });
83-83: Consistent capitalization of 'CSS'In the log message, 'Css' should be capitalized as 'CSS' for consistency.
Apply this diff:
- logger.warn("Css loaders have already been added to the packem config."); + logger.warn("CSS loaders have already been added to the packem config.");
179-187: Avoid mutating variables inside.mapfunctionReassigning
loaderinside the.mapfunction can lead to unintended side effects. It's better to avoid mutating variables within functional iterations.Apply this diff to refactor the code:
- const stringCssLoaders = cssLoaders - .map((loader) => { - if (loader === "sass-embedded" || loader === "node-sass") { - // eslint-disable-next-line no-param-reassign - loader = "sass"; - } - - return `${loader}Loader`; - }) - .join(", "); + const stringCssLoaders = cssLoaders + .map((loader) => { + const actualLoader = (loader === "sass-embedded" || loader === "node-sass") ? "sass" : loader; + return `${actualLoader}Loader`; + }) + .join(", ");
72-72: Capitalize 'TypeDoc' correctly in log messageFor consistency and proper naming, 'typedoc' should be 'TypeDoc'.
Apply this diff:
- logger.info("Adding typedoc dependencies..."); + logger.info("Adding TypeDoc dependencies...");
78-78: Capitalize 'TypeDoc' correctly in success messageMaintain consistent capitalization for 'TypeDoc' to reflect the correct product name.
Apply this diff:
- logger.success("\nTypedoc added!"); + logger.success("\nTypeDoc added!");packages/packem/src/commands/init.ts (6)
299-302: Remove Redundant ESLint Disable CommentAfter refactoring to avoid reassigning the
loaderparameter, the ESLint disable comment is no longer necessary. Removing unnecessary comments helps keep the codebase clean.You can remove the following comment:
- // eslint-disable-next-line no-param-reassign
186-186: EnsurepackageNameIs Defined Before UseIn the switch statement, there's a possibility that
packageNameremainsundefinedif an invalid value is provided foroptions.isolatedDeclarationTransformer. Although you callcancel, the code continues execution. This could lead toundefinedbeing added topackagesToInstallor cause other unexpected behavior.Consider adding an
elseblock or exiting the process aftercancelto ensurepackageNameis defined:default: { cancel("Invalid isolated declaration transformer"); + process.exit(1); }
43-43: Use aSetforpackagesToInstallto Prevent Duplicate InstallationsUsing an array for
packagesToInstallmay lead to duplicate package names, resulting in redundant installations. By using aSet, you ensure each package is unique.Apply this diff to initialize
packagesToInstallas aSet:- const packagesToInstall: string[] = []; + const packagesToInstall: Set<string> = new Set();Update
pushoperations toadd:- packagesToInstall.push("typescript@latest"); + packagesToInstall.add("typescript@latest");When installing, spread the set into an array:
- await installPackage(packagesToInstall, { cwd: options.dir, dev: true, silent: true }); + await installPackage([...packagesToInstall], { cwd: options.dir, dev: true, silent: true });
52-52: Prompt User for TypeScript Version Instead of UsinglatestAutomatically installing
typescript@latestmight introduce unexpected breaking changes. It's safer to prompt the user for the desired TypeScript version or specify a stable version range.Consider prompting the user:
const typescriptVersion = await text({ message: "Enter the TypeScript version to install (e.g., ^5.0.0 or leave empty for latest):", }); const typescriptPackage = typescriptVersion ? `typescript@${typescriptVersion}` : "typescript@latest"; packagesToInstall.add(typescriptPackage);
247-248: Check for Existing Packages Before Adding to Install ListBefore adding dependencies to
packagesToInstall, check if they are already installed to avoid unnecessary installations.Modify your loop:
for (const loader of cssLoaders) { for (const pkg of cssLoaderDependencies[loader]) { + if (!packages.includes(pkg)) { packagesToInstall.add(pkg); + } } }
381-384: Add Error Handling for Package InstallationPackage installation might fail due to network issues or other errors. Adding error handling ensures the user is informed, and the process can respond appropriately.
Wrap the installation in a try-catch block:
s.start("Installing packages"); + try { await installPackage([...packagesToInstall], { cwd: options.dir, dev: true, silent: true }); s.stop("Installed packages"); + } catch (error) { + s.stop("Failed to install packages"); + log.error("An error occurred during package installation:", error); + process.exit(1); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/test.yml(1 hunks)examples/styles/package.json(2 hunks)examples/styles/packem.config.ts(1 hunks)packages/packem/README.md(1 hunks)packages/packem/__tests__/intigration/cli.test.ts(1 hunks)packages/packem/src/commands/add.ts(4 hunks)packages/packem/src/commands/init.ts(9 hunks)packages/packem/src/commands/utils/css-loader-dependencies.ts(1 hunks)packages/packem/src/commands/utils/css-minifier-dependencies.ts(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(0 hunks)packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts(1 hunks)packages/packem/src/rollup/watch.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/packem/src/rollup/get-rollup-options.ts
✅ Files skipped from review due to trivial changes (1)
- packages/packem/src/commands/utils/css-minifier-dependencies.ts
🔇 Additional comments (10)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
20-20: LGTM!
The export statement is clean and follows conventions.
examples/styles/packem.config.ts (1)
5-8: LGTM! Import statements are well-organized.
The imports are logically grouped with CSS loaders together and the minifier separate. The addition of cssnanoMinifier is a good choice for CSS optimization.
packages/packem/src/rollup/watch.ts (3)
21-21: LGTM: Parameter list updates improve type safety.
The addition of useCache as a required parameter and explicit watcher parameter makes the function signature more robust and type-safe.
Also applies to: 29-29
181-181: LGTM: Consistent cache handling implementation.
The useCache parameter is correctly passed to watchHandler, maintaining consistency with the cache handling logic.
207-207: LGTM: Consistent cache handling for type declarations.
The useCache parameter is correctly passed to the type declaration watch handler, maintaining consistency with the bundle mode cache handling.
.github/workflows/test.yml (1)
99-99: LGTM: Install packages parameter change.
The parameter change to "false" is correct as package installation is handled in a dedicated step later in the workflow.
packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (1)
18-18: Verify PostCSS configuration loading.
The uncommented import of loadConfig is now actively used to load PostCSS configuration. Let's verify that the configuration loading works correctly across different environments.
✅ Verification successful
Let me analyze the results and generate one more verification script to check the implementation details of the loadConfig function.
PostCSS configuration loading is properly implemented
The verification shows that:
- The
loadConfigfunction is correctly implemented inload-config.tsand imported inindex.ts - It's actively used in the PostCSS loader to load configurations with proper environment and options handling
- The implementation is supported by necessary dependencies (
postcss-load-configv6.0.1) as shown in package.json - Integration tests for PostCSS configuration are present and passing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify PostCSS configuration loading implementation
# Test 1: Check if loadConfig is properly implemented
ast-grep --pattern 'export default function loadConfig($_) {
$$$
}'
# Test 2: Check for any test files related to PostCSS configuration
fd -e test.ts -e spec.ts -e test.js -e spec.js . | rg -i "postcss.*config"
# Test 3: Look for example PostCSS configurations in the codebase
rg "postcss.*config" -A 5
Length of output: 16803
Script:
#!/bin/bash
# Check the implementation of loadConfig function and its usage
ast-grep --pattern 'export default async function loadConfig($_) {
$$$
}'
# Look for the usage of loadConfig in the codebase
rg "loadConfig" -A 3
Length of output: 1560
packages/packem/__tests__/intigration/cli.test.ts (1)
334-336: LGTM: The onSuccess configuration is properly tested.
The test case correctly verifies the onSuccess command execution through configuration. The implementation follows the test suite's patterns and includes appropriate assertions.
packages/packem/README.md (1)
412-418: LGTM! Clear and well-structured plugin order documentation.
The plugin resolution order is logically organized and clearly documented, helping developers understand how their plugins will be executed in the build pipeline.
packages/packem/src/commands/init.ts (1)
Line range hint 274-280: Verify Conditional Logic for Installing CSS Minifier Dependencies
When checking whether to install the selected CSS minifier, the current condition might not correctly determine if the minifier is already included in the CSS loaders. This could lead to unnecessary prompts or missed installations.
Run the following script to confirm whether the condition correctly checks for existing dependencies:
If the pattern is not matched, consider updating the condition to:
- if (!cssLoaders.includes("lightningcss")) {
+ if (!cssLoaders.includes(cssMinifier)) {| const stringCssLoaders = cssLoaders | ||
| .map((loader) => { | ||
| if (loader === "sass-embedded" || loader === "node-sass") { | ||
| // eslint-disable-next-line no-param-reassign | ||
| loader = "sass"; | ||
| } | ||
|
|
||
| packemConfig += `${loader as string}Loader, `; | ||
| } | ||
| return `${loader}Loader`; | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid Reassigning Function Parameters to Prevent Side Effects
In the .map() function, you're reassigning the loader parameter, which can lead to unintended side effects and makes the code harder to understand. Instead, assign the modified value to a new variable.
Apply this diff to refactor the code:
const stringCssLoaders = cssLoaders
.map((loader) => {
- if (loader === "sass-embedded" || loader === "node-sass") {
- // eslint-disable-next-line no-param-reassign
- loader = "sass";
- }
- return `${loader}Loader`;
+ const normalizedLoader = (loader === "sass-embedded" || loader === "node-sass") ? "sass" : loader;
+ return `${normalizedLoader}Loader`;
})
.join(", ");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const stringCssLoaders = cssLoaders | |
| .map((loader) => { | |
| if (loader === "sass-embedded" || loader === "node-sass") { | |
| // eslint-disable-next-line no-param-reassign | |
| loader = "sass"; | |
| } | |
| packemConfig += `${loader as string}Loader, `; | |
| } | |
| return `${loader}Loader`; | |
| }) | |
| const stringCssLoaders = cssLoaders | |
| .map((loader) => { | |
| const normalizedLoader = (loader === "sass-embedded" || loader === "node-sass") ? "sass" : loader; | |
| return `${normalizedLoader}Loader`; | |
| }) |
| const cssLoaders: (keyof typeof cssLoaderDependencies | "sourceMap")[] = []; | ||
|
|
||
| if (options.css) { | ||
| cssLoaders = (await multiselect({ | ||
| const mainCssLoader = (await select({ | ||
| message: "Pick a css loader", | ||
| options: [ | ||
| { label: "PostCSS", value: "postcss" }, | ||
| { label: "Lightning CSS", value: "lightningcss" }, | ||
| ], | ||
| })) as keyof typeof cssLoaderDependencies; |
There was a problem hiding this comment.
Handle Case Where No CSS Loader Is Selected
If the user cancels the prompt or doesn't select a CSS loader, mainCssLoader could be undefined, leading to errors when pushed to cssLoaders.
Add a check to handle undefined:
const mainCssLoader = (await select({
message: "Pick a css loader",
options: [
{ label: "PostCSS", value: "postcss" },
{ label: "Lightning CSS", value: "lightningcss" },
],
})) as keyof typeof cssLoaderDependencies;
+ if (!mainCssLoader) {
+ cancel("No CSS loader selected.");
+ process.exit(1);
+ }
cssLoaders.push(mainCssLoader);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
1-18: Consider adding TypeScript types and documentation.While the structure is clean and well-organized, consider these improvements for better maintainability:
- Add TypeScript interface for type safety
- Include JSDoc documentation explaining the purpose and usage
+/** Interface defining the structure of CSS loader dependencies */ +interface CssLoaderDependenciesMap { + [key: string]: string[]; +} + +/** + * Maps CSS preprocessors to their required dependencies. + * Used by the packem add command to install necessary packages. + */ const cssLoaderDependencies = { less: ["less"], lightningcss: ["lightningcss"],examples/styles/package.json (1)
21-21: Consider using caret version range for consistency.The
cssnanopackage is pinned to exact version7.0.6while other dependencies use caret ranges. Consider using^7.0.6for consistency with other dependencies unless there's a specific reason for version pinning.packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (2)
Line range hint
43-279: Consider refactoring the process method for better maintainability.The process method is quite complex and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Configuration processing
- Plugin setup
- CSS processing
- Message handling
- Output generation
Example refactor for the configuration processing part:
async function setupPostCSSConfig(options: PostCSSOptions, id: string, cwd: string, environment: string) { const config = await loadConfig(id, cwd, environment, options.config); return { ...config.options, ...options, from: id, map: { annotation: false, inline: false, prev: mm(map).relative(dirname(id)).toObject(), sourcesContent: sourceMap ? sourceMap.content : true, }, plugins: [], to: options.to ?? id, }; }
Line range hint
43-279: Add error handling for configuration loading.The configuration loading could fail silently. Consider adding explicit error handling:
const config = await loadConfig(this.id, this.cwd as string, this.environment, this.options.config) .catch((error) => { throw new Error(`Failed to load PostCSS config: ${error.message}`); });packages/packem/__tests__/intigration/cli.test.ts (1)
Line range hint
1-1: Fix typo in the file path.The directory name contains a typo: "intigration" should be "integration".
packages/packem/README.md (2)
412-418: Consider enhancing the plugin order documentationWhile the plugin order is clearly listed, it would be helpful to add a brief explanation of why this order matters and how it affects the build process. This context would help users make informed decisions about plugin placement.
Consider adding a note like:
The resolved plugins will be in the following order: - Alias - User plugins with enforce: 'pre' - Rollup core plugins - User plugins without enforce value - Rollup build plugins - User plugins with enforce: 'post' - Rollup post build plugins (minify, manifest, copy, reporting) + +> Note: This order ensures that aliases are resolved first, followed by pre-processing plugins, +> core functionality, and finally post-processing tasks like minification. Understanding this order +> is crucial when plugins need to interact with or depend on each other's output.
Line range hint
1-24: Consider adding a table of contentsThe documentation is comprehensive and well-structured. To improve navigation and user experience, consider adding a table of contents at the beginning of the document. This would help users quickly find specific sections, especially given the extensive nature of the documentation.
Consider adding something like:
## Table of Contents - [Overview](#overview) - [Features](#features) - [Installation](#install) - [Usage](#usage) - [Initialize Configuration](#initialize-the-packem-configuration) - [Bundle Files](#bundle-files) - [Plugins](#plugins) - [CSS Processing](#css-and-css-modules) - [Configuration](#configuration) - [Related Projects](#related)packages/packem/src/commands/add.ts (3)
30-40: Improve error handling and user experience.The error handling for locating the Packem configuration file is a good improvement. However, consider enhancing it further:
- Provide a more informative error message, guiding the user on the next steps.
- Consider offering an option to automatically run
packem initif the configuration file is not found, streamlining the user experience.Here's a suggested improvement:
try { packemConfigFilePath = await findPackemFile(rootDirectory, options.config); } catch (error) { logger.error("Could not find a Packem configuration file."); const runInit = await confirm({ message: "Would you like to run `packem init` to create a new configuration file?", }); if (runInit) { // Run the `packem init` command // You can extract the logic from the `init` command and invoke it here } else { logger.info("Please run `packem init` manually to create a new configuration file."); return; } }
43-47: Consider using a more robust method to determine the configuration format.The current approach of checking for the presence of the
importkeyword to determine the configuration format (CommonJS or ESM) may not be reliable in all cases. For example, if the configuration file usesimportstatements for other purposes or if it uses dynamicimport()expressions, the format detection might be inaccurate.Consider using a more robust method to determine the configuration format, such as:
- Checking the file extension (
.jsfor CommonJS,.mjsfor ESM).- Parsing the configuration file's AST (Abstract Syntax Tree) to analyze the syntax and module system used.
- Utilizing a configuration file format that explicitly specifies the module system (e.g.,
packem.config.cjsfor CommonJS,packem.config.mjsfor ESM).Here's an example using file extension checking:
let packemConfigFormat = "cjs"; if (packemConfigFilePath.endsWith(".mjs")) { packemConfigFormat = "esm"; }
72-79: Improve logging and error handling for TypeDoc dependency installation.The logging and spinner usage for installing TypeDoc dependencies is a good addition. However, consider enhancing it further:
- Log any errors that occur during the installation process.
- Handle cases where the installation fails and provide appropriate error messages to the user.
- Consider moving the success message to be displayed only after the configuration file has been successfully updated.
Here's a suggested improvement:
try { s.start("Installing TypeDoc packages..."); await installPackage(typedocPackages, { cwd: process.cwd(), dev: true, silent: true }); s.stop("TypeDoc packages installed successfully."); } catch (error) { s.stop("Failed to install TypeDoc packages."); logger.error("An error occurred while installing TypeDoc dependencies:"); logger.error(error); return; } // Update the configuration file // ... logger.success("TypeDoc added successfully!");packages/packem/src/commands/init.ts (2)
8-9: Refactor to use a single CSS dependencies file.Consider consolidating the CSS loader and minifier dependencies into a single file to simplify imports and maintain a unified structure for CSS-related dependencies.
-import cssLoaderDependencies from "./utils/css-loader-dependencies"; -import type cssMinifierDependencies from "./utils/css-minifier-dependencies"; +import { cssLoaderDependencies, cssMinifierDependencies } from "./utils/css-dependencies";
52-52: Consider pinning the TypeScript version.Instead of installing the latest version, consider specifying a specific TypeScript version to ensure consistency across different environments and to avoid potential breaking changes.
-packagesToInstall.push("typescript@latest"); +packagesToInstall.push("typescript@4.9.5");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
examples/styles/package.json(2 hunks)examples/styles/packem.config.ts(1 hunks)packages/packem/README.md(1 hunks)packages/packem/__tests__/intigration/cli.test.ts(1 hunks)packages/packem/src/commands/add.ts(4 hunks)packages/packem/src/commands/init.ts(9 hunks)packages/packem/src/commands/utils/css-loader-dependencies.ts(1 hunks)packages/packem/src/commands/utils/css-minifier-dependencies.ts(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(0 hunks)packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts(1 hunks)packages/packem/src/rollup/watch.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/packem/src/rollup/get-rollup-options.ts
✅ Files skipped from review due to trivial changes (1)
- packages/packem/src/commands/utils/css-minifier-dependencies.ts
🔇 Additional comments (22)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
20-20: LGTM!
The default export is appropriate for this module.
examples/styles/packem.config.ts (2)
5-8: LGTM! Well-organized imports
The imports are now logically grouped and the addition of cssnanoMinifier aligns with the enhanced CSS processing capabilities.
15-16: Verify CSS loader processing order
The order of CSS loaders has been updated. Please confirm that this order (postcssLoader, stylusLoader, lessLoader, sassLoader, sourceMapLoader) is intentional and optimal for your CSS processing pipeline. The order of loaders can affect how the CSS is processed.
The addition of cssnanoMinifier is a good enhancement for optimizing the final CSS output.
Let's verify if this loader order is consistent across other configurations:
✅ Verification successful
Let me check the loader configurations in the other files to get a complete picture.
Let me try a different approach to find the loader configurations.
CSS loader order is project-specific and correctly configured
The loader configuration in examples/styles/packem.config.ts is intentionally different from other configs as it's specifically designed to demonstrate various CSS preprocessor capabilities:
styles/packem.config.ts: Full preprocessor stack with all loadersreact-tailwindcss/packem.config.ts: Tailwind-specific with just PostCSSreact-tsx-css-module/packem.config.ts: CSS modules setup with PostCSS and sourcemaps
The order in styles/packem.config.ts is correct as:
- PostCSS first handles modern CSS features
- Preprocessor loaders (Stylus, Less, Sass) handle their respective syntaxes
- Source map generation comes last, which is the logical position
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other packem configurations to ensure consistent loader ordering
# Expected: Similar loader ordering in other config files
# Search for other packem config files with CSS loader configurations
rg -l "css.*loader" --glob "packem.config.*"
# Search for specific loader order patterns
rg "loaders.*postcssLoader.*stylusLoader.*lessLoader.*sassLoader" --glob "packem.config.*"
Length of output: 390
Script:
#!/bin/bash
# Get the loader configurations from all identified config files
echo "=== styles/packem.config.ts ==="
rg "loaders.*loader" examples/styles/packem.config.ts -A 2
echo -e "\n=== react-tailwindcss/packem.config.ts ==="
rg "loaders.*loader" examples/react-tailwindcss/packem.config.ts -A 2
echo -e "\n=== react-tsx-css-module/packem.config.ts ==="
rg "loaders.*loader" examples/react-tsx-css-module/packem.config.ts -A 2
Length of output: 488
Script:
#!/bin/bash
# Search for loader arrays in config files with more context
echo "=== styles/packem.config.ts ==="
rg "loaders:" examples/styles/packem.config.ts -B 2 -A 2
echo -e "\n=== react-tailwindcss/packem.config.ts ==="
rg "loaders:" examples/react-tailwindcss/packem.config.ts -B 2 -A 2
echo -e "\n=== react-tsx-css-module/packem.config.ts ==="
rg "loaders:" examples/react-tsx-css-module/packem.config.ts -B 2 -A 2
Length of output: 937
examples/styles/package.json (1)
21-21: Verify the integration of new CSS-related dependencies.
The added dependencies align with the PR's objective of enhancing CSS processing. Let's verify their integration:
Also applies to: 23-23, 35-35
✅ Verification successful
CSS-related dependencies are properly integrated and utilized across the codebase
The verification shows comprehensive integration of the CSS-related dependencies:
cssnanois properly utilized in multiple configuration files and core implementation files- CSS loaders and minifiers are correctly imported and configured in various
packem.config.tsfiles - The dependencies are well-integrated into the build system's CSS processing pipeline, with proper type definitions and test coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of newly added CSS-related dependencies
# Test 1: Check if cssnano is imported/used
echo "Checking cssnano usage:"
rg -l "cssnano" --type ts
# Test 2: Check if icss-utils is imported/used
echo -e "\nChecking icss-utils usage:"
rg -l "icss-utils" --type ts
# Test 3: Check if stylus is imported/used
echo -e "\nChecking stylus usage:"
rg -l "stylus" --type ts
# Test 4: Check for related configuration
echo -e "\nChecking for CSS-related configuration:"
fd -e config.ts -x cat {} \; | rg -A 5 "css|CSS|style"
Length of output: 3677
packages/packem/src/rollup/watch.ts (3)
181-182: LGTM: Proper implementation of required useCache parameter
The useCache parameter is correctly passed based on the context configuration.
207-208: LGTM: Consistent usage of useCache for DTS generation
The useCache parameter is consistently applied to both bundle and DTS generation.
21-30: Breaking Change: useCache parameter is now required
Making useCache a required parameter is a breaking change that affects all callers of watchHandler. While this improves type safety by making caching decisions explicit, ensure all call sites are updated accordingly.
Let's verify all call sites:
✅ Verification successful
useCache parameter is already provided at all call sites
The verification shows that both call sites of watchHandler in packages/packem/src/rollup/watch.ts already pass the useCache parameter:
- Lines 174-183: First call with
mode: "bundle" - Lines 203-209: Second call with
mode: "types"
Therefore, making useCache required is a safe change that aligns with the actual usage in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to watchHandler to ensure they pass useCache
ast-grep --pattern 'watchHandler({$$$})'
Length of output: 1075
packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (1)
18-18: LGTM: Import change for configuration loading.
The uncommented import of loadConfig aligns with the loader's functionality for processing PostCSS configurations.
packages/packem/__tests__/intigration/cli.test.ts (1)
Line range hint 1-424: Test suite structure looks good!
The test suite demonstrates good practices:
- Proper test isolation using temporary directories
- Consistent cleanup in beforeEach/afterEach
- Clear assertions and error messages
- Helper functions for common setup tasks
packages/packem/src/commands/add.ts (4)
81-207: Thoroughly test the CSS loader and minifier functionality.
The addition of CSS loader and minifier support is a significant enhancement. To ensure its reliability and compatibility, conduct thorough testing:
- Test various combinations of CSS loaders and minifiers to ensure they work as expected.
- Verify that the generated configuration file is valid and includes the selected loaders and minifier correctly.
- Ensure that the installed dependencies are compatible with each other and with the project's existing dependencies.
- Test the CSS processing functionality with different types and sizes of CSS files to ensure proper handling and minification.
Create a comprehensive test suite to cover the CSS loader and minifier functionality. Here's a high-level outline:
describe("CSS Loader and Minifier", () => {
it("should add the selected CSS loaders to the configuration", async () => {
// Test different combinations of CSS loaders
// Verify that the configuration file is updated correctly
});
it("should add the selected CSS minifier to the configuration", async () => {
// Test different CSS minifiers
// Verify that the configuration file is updated correctly
});
it("should install the required CSS dependencies", async () => {
// Verify that the selected CSS loaders and minifier dependencies are installed
});
it("should process and minify CSS files correctly", async () => {
// Test with different types and sizes of CSS files
// Verify that the processed and minified CSS files are generated correctly
});
// Add more test cases as needed
});58-62: Ensure consistent handling of TypeDoc dependencies.
The handling of TypeDoc dependencies has been modified to check the configuration format and prepend the appropriate import statement. However, ensure that this change is consistently applied throughout the codebase and does not introduce any inconsistencies or break existing functionality.
Run the following script to verify the consistency of TypeDoc dependency handling:
#!/bin/bash
# Description: Verify consistent handling of TypeDoc dependencies.
# Test: Check if the TypeDoc import statement is added correctly for CommonJS format.
# Expect: TypeDoc import statement for CommonJS format is found.
if ! grep -q 'magic\.prepend(`const typedocBuilder = require("@visulima/packem/builder/typedoc");\\n`);' ./packages/packem/src/commands/add.ts; then
echo "Error: TypeDoc import statement for CommonJS format not found in add.ts."
exit 1
fi
# Test: Check if the TypeDoc import statement is added correctly for ESM format.
# Expect: TypeDoc import statement for ESM format is found.
if ! grep -q 'magic\.prepend(`import typedocBuilder from "@visulima/packem/builder/typedoc";\\n`);' ./packages/packem/src/commands/add.ts; then
echo "Error: TypeDoc import statement for ESM format not found in add.ts."
exit 1
fi
# Test: Check if the TypeDoc builder configuration is added correctly.
# Expect: TypeDoc builder configuration is found.
if ! grep -q 'magic\.replace("builder: {", `builder: {\\n typedoc: typedocBuilder,\\n `);' ./packages/packem/src/commands/add.ts; then
echo "Warning: TypeDoc builder configuration not found in add.ts."
fi4-4: Ensure the new dependencies are properly installed and available.
Verify that the @clack/prompts package is installed and available in the project's dependencies. If not already present, consider adding it to the project's package.json and running npm install or yarn install to ensure its availability.
Run the following script to verify the dependency:
✅ Verification successful
@clack/prompts dependency is properly configured
The package @clack/prompts version 0.7.0 is listed in the dependencies, and this is a monorepo setup with workspaces configured in the root package.json. The import can be safely used in the code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify @clack/prompts is installed and available.
# Test: Check if @clack/prompts is listed in package.json dependencies or devDependencies.
# Expect: @clack/prompts is found in package.json.
if ! grep -q '"@clack/prompts"' ./package.json; then
echo "Error: @clack/prompts not found in package.json dependencies."
exit 1
fi
# Test: Check if @clack/prompts is installed in node_modules.
# Expect: @clack/prompts is found in node_modules.
if ! fd -q node_modules/@clack/prompts; then
echo "Warning: @clack/prompts not found in node_modules. Run npm install or yarn install."
fi
Length of output: 185
Script:
#!/bin/bash
# Let's check the package.json files in all packages since this seems to be a monorepo
fd package.json --type f --exec cat {} \; | grep -A2 -B2 "@clack/prompts"
# Also check if there are any workspace dependencies defined
fd "package.json" --type f --exec grep -l "workspaces" {} \;
Length of output: 358
11-12: Ensure the new utility constants are properly imported and used.
Verify that the cssLoaderDependencies and cssMinifierDependencies constants are correctly imported from their respective utility files and used appropriately within the createAddCommand function.
Run the following script to verify the imports and usage:
✅ Verification successful
The imports are correctly used throughout the codebase
The verification shows that both imports are properly used:
cssLoaderDependenciesis used for type checking and accessing loader dependencies in the CSS configuration sectioncssMinifierDependenciesis used as a type for CSS minifier selection and configuration
Both utility files exist at the correct paths and their imports are actively used in the code, particularly in the CSS configuration section of the createAddCommand function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify cssLoaderDependencies and cssMinifierDependencies are imported and used correctly.
# Test: Check if cssLoaderDependencies is imported.
# Expect: import statement for cssLoaderDependencies is found.
if ! grep -q 'import cssLoaderDependencies from "./utils/css-loader-dependencies"' ./packages/packem/src/commands/add.ts; then
echo "Error: cssLoaderDependencies import not found in add.ts."
exit 1
fi
# Test: Check if cssMinifierDependencies is imported.
# Expect: import statement for cssMinifierDependencies is found.
if ! grep -q 'import type cssMinifierDependencies from "./utils/css-minifier-dependencies"' ./packages/packem/src/commands/add.ts; then
echo "Error: cssMinifierDependencies import not found in add.ts."
exit 1
fi
# Test: Check if cssLoaderDependencies is used.
# Expect: usage of cssLoaderDependencies is found.
if ! grep -q 'cssLoaderDependencies\[' ./packages/packem/src/commands/add.ts; then
echo "Warning: cssLoaderDependencies usage not found in add.ts."
fi
# Test: Check if cssMinifierDependencies is used.
# Expect: usage of cssMinifierDependencies is found.
if ! grep -q 'cssMinifierDependencies\[' ./packages/packem/src/commands/add.ts; then
echo "Warning: cssMinifierDependencies usage not found in add.ts."
fi
Length of output: 580
Script:
#!/bin/bash
# Let's check the content and usage more thoroughly
# First, let's see the full content of add.ts
echo "=== Content of add.ts ==="
cat ./packages/packem/src/commands/add.ts
# Then check if the utility files exist
echo -e "\n=== Checking utility files ==="
ls -l ./packages/packem/src/commands/utils/css-loader-dependencies.* 2>/dev/null
ls -l ./packages/packem/src/commands/utils/css-minifier-dependencies.* 2>/dev/null
# Let's also check for any usage patterns with more context
echo -e "\n=== Checking usage patterns ==="
rg -A 3 "cssLoaderDependencies" ./packages/packem/src/commands/add.ts
rg -A 3 "cssMinifierDependencies" ./packages/packem/src/commands/add.ts
Length of output: 12247
packages/packem/src/commands/init.ts (9)
43-44: LGTM!
The change to accumulate packages in an array before installation is a good approach for better organization and consistency in package management.
128-128: LGTM!
The change to conditionally push the appropriate transformer package based on the selected option is correct and maintains consistency with the rest of the code.
186-186: LGTM!
The change to push the selected isolated declaration transformer package to the packagesToInstall array is correct and aligns with the overall package installation approach.
Line range hint 213-240: LGTM!
The changes to handle the selection of extra CSS loaders and the special case for the Sass loader are implemented correctly. The approach of filtering out the generic "sass" loader and replacing it with the specific Sass loader variant chosen by the user is a good way to handle this scenario.
247-249: LGTM!
The change to iterate over the cssLoaders array and push the corresponding dependencies from cssLoaderDependencies to packagesToInstall is implemented correctly and aligns with the overall package installation approach.
297-308: LGTM!
The change to map the cssLoaders array to a string of loader names, handling the special cases for "sass-embedded" and "node-sass", and joining them with commas is implemented correctly. This approach ensures that the correct loader names are used in the generated configuration.
381-385: LGTM!
The change to conditionally install packages only if there are packages in the packagesToInstall array is a good optimization. The usage of the spinner to provide feedback during the installation process enhances the user experience.
200-211: Verify that the selected main CSS loader is not already included in the extra loaders.
To prevent duplicate loader installations, ensure that the selected main CSS loader is not present in the extraCssLoaders array before prompting for installation.
✅ Verification successful
No duplication issue found between main CSS loader and extra loaders
The code is correctly handling the CSS loaders without any risk of duplication:
- The main CSS loader (
mainCssLoader) is selected from PostCSS/Lightning CSS options - Extra loaders (
extraCssLoaders) are selected from a different set of options (Sass/Stylus/Less) - These sets are mutually exclusive, making duplication impossible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the selected main CSS loader is not included in the extra loaders.
# Test: Search for the usage of `extraCssLoaders` and `cssLoaders`. Expect: No occurrences of adding the main loader to `extraCssLoaders` before merging with `cssLoaders`.
rg -A 5 $'extraCssLoaders\\.push\\(mainCssLoader\\)'
Length of output: 50
Script:
#!/bin/bash
# Let's search for the definition and usage of extraCssLoaders to understand the context better
rg -A 10 "extraCssLoaders.*=" packages/packem/src/commands/init.ts
# Also search for any push operations on extraCssLoaders
rg -A 5 "extraCssLoaders.*push" packages/packem/src/commands/init.ts
# Let's see how cssLoaders and extraCssLoaders are used together
rg -A 10 "cssLoaders.*=" packages/packem/src/commands/init.ts
Length of output: 1826
280-280: Verify that the selected CSS minifier is not already included in the CSS loaders.
To prevent duplicate package installations, ensure that the selected CSS minifier is not already present in the cssLoaders array before prompting for installation.
✅ Verification successful
Based on the code context, I can now generate the final response as the verification is complete. The code shows that there is already a check in place:
The code already handles duplicate CSS minifier installations correctly
The code already includes a check if (!cssLoaders.includes("lightningcss")) before adding the CSS minifier to packagesToInstall. This prevents duplicate installation when Lightning CSS is selected as both the loader and minifier. For CSSNano, no such check is needed as it can only be a minifier.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the selected CSS minifier is not included in the CSS loaders.
# Test: Search for the usage of `cssLoaders` and `cssMinifier`. Expect: No occurrences of the selected minifier in `cssLoaders` when prompting for installation.
rg -A 5 $'cssLoaders\\.includes\\(cssMinifier\\)'
Length of output: 47
Script:
#!/bin/bash
# Let's first understand the context around cssMinifier and cssLoaders
rg -A 10 "cssMinifier|cssLoaders" packages/packem/src/commands/init.ts
Length of output: 6593
| postcss: [ | ||
| "postcss", | ||
| "postcss-load-config", | ||
| "postcss-modules-extract-imports", | ||
| "postcss-modules-local-by-default", | ||
| "postcss-modules-scope", | ||
| "postcss-modules-values", | ||
| "postcss-value-parser", | ||
| "icss-utils", | ||
| ], |
There was a problem hiding this comment.
💡 Codebase verification
PostCSS dependencies list needs to be updated
The current PostCSS dependencies list is missing some commonly used plugins that are present in the example projects and test fixtures:
autoprefixer: Used in react-tailwindcss examplepostcss-custom-properties: Used in modules-duplication testsugarss: Referenced as a parser in postcss-config test
These dependencies should be considered for inclusion in the default set as they represent real usage patterns in the project's examples and tests.
🔗 Analysis chain
Verify PostCSS dependencies completeness.
The PostCSS dependencies list includes essential modules, but let's verify if any commonly used plugins are missing from the default set.
Let's examine the content of these package.json files to see what PostCSS dependencies they use.
Let's check the content of the PostCSS config files to see if they use any additional plugins that should be included in the default dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for PostCSS-related dependencies in example projects and configs
rg -l "postcss" examples/ -g "package.json"
Length of output: 153
Script:
#!/bin/bash
# Check PostCSS-related dependencies in the found package.json files
echo "=== styles/package.json ==="
cat examples/styles/package.json | jq '.dependencies,.devDependencies' 2>/dev/null
echo -e "\n=== react-tailwindcss/package.json ==="
cat examples/react-tailwindcss/package.json | jq '.dependencies,.devDependencies' 2>/dev/null
echo -e "\n=== react-tsx-css-module/package.json ==="
cat examples/react-tsx-css-module/package.json | jq '.dependencies,.devDependencies' 2>/dev/null
# Also let's check if there are any PostCSS config files
echo -e "\n=== PostCSS Config Files ==="
fd -e js -e cjs -e json "postcss.config"
Length of output: 2475
Script:
#!/bin/bash
# Check PostCSS config files content
echo "=== react-tailwindcss/postcss.config.js ==="
cat examples/react-tailwindcss/postcss.config.js
echo -e "\n=== modules-duplication/postcss.config.js ==="
cat packages/packem/__fixtures__/css/modules-duplication/postcss.config.js
echo -e "\n=== postcss-config/src/postcss.config.js ==="
cat packages/packem/__fixtures__/css/postcss-config/src/postcss.config.js
Length of output: 730
| @@ -332,7 +332,7 @@ export { a }; | |||
| await createPackemConfig(temporaryDirectoryPath, { | |||
| config: { | |||
| onSuccess: "echo hello && echo world", | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation found in packem.ts, I can now generate the final response:
Security and cross-platform concerns confirmed in onSuccess implementation
The current implementation of onSuccess has security vulnerabilities and cross-platform compatibility issues:
- Commands are executed with
shell: truewithout any sanitization - No command validation or allowlist system is implemented
- The
timeoutparameter (30s default) is the only safety measure - Direct shell command execution may fail on Windows due to shell differences
Recommendations:
- Add command validation/sanitization before execution
- Use cross-platform command execution (e.g.,
cross-spawn) - Document security implications in README
- Add test cases for command injection and failed executions
🔗 Analysis chain
Consider security and cross-platform compatibility in onSuccess tests.
The test cases for the onSuccess command have potential issues:
-
Security: Executing shell commands directly from config/CLI without validation could be dangerous. Consider adding tests to verify command sanitization.
-
Cross-platform: The command
echo hello && echo worldmight not work on Windows. Consider using cross-platform alternatives or mocking the command execution. -
Test coverage: Add test cases for:
- Failed commands
- Command injection attempts
- Verifying that onSuccess only runs after successful builds
Let's check if there are any command validation or sanitization measures:
Consider:
- Using a cross-platform command execution library
- Implementing a command allowlist/validation system
- Adding documentation about security implications
Let me search for more specific implementation details about the onSuccess command handling.
Let me search for the specific implementation of the onSuccess handler to understand how commands are executed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for command validation in the codebase
rg -A 5 'onSuccess.*command'
Length of output: 1363
Script:
#!/bin/bash
# Search for onSuccess implementation
ast-grep --pattern 'onSuccess'
# Also search for command execution related code
rg -A 10 'exec.*command|spawn.*command'
Length of output: 4627
Script:
#!/bin/bash
# Search for files containing onSuccess implementation
fd -e ts -e js -e mjs onSuccess
# Search for the actual implementation
rg -A 10 "function.*onSuccess|const.*onSuccess"
Length of output: 2312
| const s = spinner(); | ||
|
|
There was a problem hiding this comment.
Ensure the spinner is properly stopped and cleaned up.
The spinner is started using spinner() from @clack/prompts, but there is no corresponding stop() call to stop the spinner when the command execution is complete or encounters an error. This may lead to the spinner continuing to run indefinitely.
Consider adding a finally block to stop the spinner after the command execution, regardless of whether it succeeds or encounters an error. For example:
try {
// Command execution logic
} catch (error) {
// Error handling
} finally {
s.stop();
}
packages/packem/src/commands/add.ts
Outdated
| s.start("Installing packages"); | ||
| await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true }); | ||
| s.stop("Installed packages"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle package installation errors gracefully.
While the package installation process is wrapped in a spinner to indicate progress, it's important to handle any errors that may occur during the installation gracefully. If an error occurs, the spinner should be stopped, and an appropriate error message should be logged to inform the user about the failure.
Here's an example of how you can handle package installation errors:
try {
s.start("Installing packages");
await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
s.stop("Packages installed successfully");
} catch (error) {
s.stop("Package installation failed");
logger.error("An error occurred while installing packages:");
logger.error(error);
// Optionally, you can provide instructions on how to manually install the packages
logger.info("Please try installing the packages manually using the following command:");
logger.info(`npm install ${packagesToInstall.join(" ")}`);
return;
}By catching any errors that occur during package installation, you can provide a better user experience by informing them about the failure and suggesting possible solutions.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (18)
packages/packem/src/commands/utils/css-loader-dependencies.ts (2)
1-20: Add TypeScript type definitions for better type safety.Consider adding TypeScript interfaces to improve type safety and documentation.
+type CssLoaderDependency = { + [key: string]: string[]; +}; + -const cssLoaderDependencies = { +const cssLoaderDependencies: CssLoaderDependency = {
1-1: Add JSDoc documentation for the module.Consider adding documentation to explain:
- The purpose of this mapping
- How it's used in the build process
- How to extend or modify the dependencies
+/** + * Maps CSS preprocessors and tools to their required dependencies. + * Used by the Packem build system to automatically install necessary packages + * when specific CSS processing features are enabled. + * + * @example + * ```ts + * import cssLoaderDependencies from './css-loader-dependencies'; + * + * // Get dependencies for sass + * const sassDeps = cssLoaderDependencies.sass; // ['sass'] + * ``` + */ const cssLoaderDependencies = {packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (5)
Line range hint
43-47: Consider adding JSDoc documentation for better maintainability.The process method is complex with multiple options. Adding JSDoc documentation would help future maintainers understand:
- The purpose of each parameter in the destructured object
- The expected return type
- Key processing steps
Example documentation:
/** * Processes CSS content with PostCSS plugins and handles various transformations * @param {Object} params - Processing parameters * @param {string} params.code - The CSS code to process * @param {any} params.extracted - Extracted CSS data * @param {any} params.map - Source map data * @returns {Promise<{code: string, extracted: any, map: any, meta: any}>} */ async process({ code, extracted, map })
Line range hint
48-103: Enhance error handling for plugin configuration.While the plugin setup is comprehensive, consider adding more robust error handling:
- Validate plugin configurations before processing
- Add specific error messages for common configuration issues
- Consider adding debug logging for plugin initialization
Example enhancement:
const validatePluginConfig = (plugins: AcceptedPlugin[]) => { for (const plugin of plugins) { if (!plugin) { throw new Error('Invalid plugin configuration: plugin cannot be null/undefined'); } } }; // Use before processing validatePluginConfig(plugins);
Line range hint
182-191: Enhance warning messages for module exports.The current warning message could be more informative by including:
- The reason for the name change
- The potential impact of the change
Example enhancement:
- this.warn(`Exported \`${name}\` as \`${newName}\` in ${relative(this.cwd as string, this.id)}`); + this.warn( + `Exported CSS module class \`${name}\` was renamed to \`${newName}\` to ensure valid JavaScript identifier in ` + + `${relative(this.cwd as string, this.id)}. This might affect your CSS module imports.` + );
Line range hint
215-262: Consider extracting injection logic to a separate module.The injection logic is complex and could benefit from being moved to a dedicated module for better maintainability and testing.
Consider creating a separate module like:
// inject-handler.ts export class InjectionHandler { constructor(private options: InjectOptions) {} generateInjectionCode(cssVar: string, modulesExports: Record<string, string>): string { // Move injection logic here } }
Line range hint
264-315: Optimize output string generation.The current string concatenation approach could be optimized for better performance with large CSS files.
Consider using string buffers or arrays:
class OutputBuilder { private parts: string[] = []; add(part: string): this { this.parts.push(part); return this; } toString(): string { return this.parts.join('\n'); } }packages/packem/__tests__/intigration/cli.test.ts (1)
Directory name "intigration" should be renamed to "integration"
The typo exists in the directory path:
packages/packem/__tests__/intigration/
- Rename the directory from
packages/packem/__tests__/intigration/topackages/packem/__tests__/integration/- Update any imports or file references accordingly
🔗 Analysis chain
Line range hint
1-1: Fix typo in directory name: "intigration" should be "integration".The directory name contains a typo that should be corrected for better project organization and maintainability.
Let's verify if this typo exists in the directory structure:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the typo exists in other files/paths fd -t d 'intigration'Length of output: 60
packages/packem/README.md (2)
419-419: Consider adding examples for each plugin type.While the plugin ordering is well documented, it would be helpful to provide concrete examples for each type of plugin (pre, core, build, post) to help users better understand when to use each enforcement level.
419-419: Enhance the attribution formatting.The attribution comment for the CSS documentation could be more visible and properly formatted. Consider using a more prominent format:
-<!-- Modified copy of https://github.com/Anidetrix/rollup-plugin-styles/blob/main/README.md --> +## Credits + +The CSS documentation section is adapted from [rollup-plugin-styles](https://github.com/Anidetrix/rollup-plugin-styles/blob/main/README.md) with modifications.packages/packem/src/commands/add.ts (7)
45-47: Use a more reliable method to determine the configuration formatCurrently, the code checks if the string
"import"is present in the configuration to determine if it's an ESM module. This could lead to false positives if the word "import" appears in comments or strings.Consider using the file extension or a more robust method to detect the module format. For example:
-let packemConfigFormat = "cjs"; -if (packemConfig.includes("import")) { +const isESM = packemConfigFilePath.endsWith(".mjs") || packemConfigFilePath.endsWith(".mts") || packemConfigFilePath.endsWith(".js") || packemConfigFilePath.endsWith(".ts"); +if (isESM) { packemConfigFormat = "esm"; }
24-24: Consider refactoring to reduce cognitive complexityThe
executefunction has high cognitive complexity, as indicated by the disabled ESLint rulesonarjs/cognitive-complexity. This can make the code harder to read and maintain.Consider refactoring the function by extracting smaller helper functions for distinct tasks, such as handling CSS loaders, managing Typedoc integration, and configuring the Packem file.
26-26: Use a more descriptive variable name for the spinnerThe variable
sis not descriptive, which can reduce code readability. Consider renaming it to something more meaningful likespinnerInstanceorloadingSpinner.Apply this diff to rename the variable:
-const s = spinner(); +const loadingSpinner = spinner();And update all occurrences of
saccordingly.
35-35: Implement user prompt to runpackem initif config is missingThere's a TODO comment suggesting adding a prompt to run
packem initwhen the Packem config file is not found. Implementing this feature would improve user experience by guiding them through the setup process.Would you like assistance in implementing this feature? I can help by providing sample code for the prompt.
131-133: Ensure 'packagesToInstall' contains unique package namesWhen adding dependencies to
packagesToInstall, duplicates may occur if multiple loaders share common dependencies. Installing duplicate packages can slow down the installation process.Consider de-duplicating
packagesToInstallbefore installing the packages.Apply this diff:
for (const loader of cssLoaders) { packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies]); } +// Remove duplicate packages +packagesToInstall = [...new Set(packagesToInstall)];
149-153: Set default value for CSS minification confirmationThe confirmation prompt for CSS minification lacks a default value. Setting
initialValuehelps streamline the user experience.Apply this diff to set a default value:
const useCssMinifier = (await confirm({ - initialValue: false, + initialValue: true, message: "Do you want to minify your css?", })) as boolean;This sets the default response to "Yes," encouraging best practices for production builds.
178-187: Handle loader name conversion consistentlyIn the mapping function constructing
stringCssLoaders, the loader names are modified for certain cases like"sass-embedded"and"node-sass". Ensure consistency in how loader names are handled throughout the code.Consider extracting the loader name normalization into a separate function to avoid repetition and potential errors.
packages/packem/src/commands/init.ts (1)
Line range hint
43-385: Prevent duplicate package installations by de-duplicating the package listThe
packagesToInstallarray may contain duplicate package names if the same package is added in multiple places. This can lead to unnecessary installation attempts and slow down the setup process. Consider de-duplicating the array before installing the packages.You can apply this change to de-duplicate the package list:
+ const uniquePackagesToInstall = [...new Set(packagesToInstall)]; if (packagesToInstall.length > 0) { s.start("Installing packages"); - await installPackage(packagesToInstall, { cwd: options.dir, dev: true, silent: true }); + await installPackage(uniquePackagesToInstall, { cwd: options.dir, dev: true, silent: true }); s.stop("Installed packages"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/test.yml(1 hunks)examples/styles/package.json(2 hunks)examples/styles/packem.config.ts(1 hunks)packages/packem/README.md(1 hunks)packages/packem/__tests__/intigration/cli.test.ts(1 hunks)packages/packem/src/commands/add.ts(4 hunks)packages/packem/src/commands/init.ts(9 hunks)packages/packem/src/commands/utils/css-loader-dependencies.ts(1 hunks)packages/packem/src/commands/utils/css-minifier-dependencies.ts(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(0 hunks)packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts(1 hunks)packages/packem/src/rollup/watch.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/packem/src/rollup/get-rollup-options.ts
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/test.yml
- packages/packem/src/commands/utils/css-minifier-dependencies.ts
🔇 Additional comments (11)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
2-17: Consider adding version constraints and verifying dependency completeness.
While the dependencies list is comprehensive, consider:
- Adding version constraints to prevent compatibility issues
- Verifying if any peer dependencies are missing
examples/styles/packem.config.ts (1)
16-16: LGTM: CSS minifier configuration.
The addition of cssnanoMinifier is well-structured and aligns with the project's goal of enhancing CSS processing capabilities.
examples/styles/package.json (2)
21-21: LGTM! New dependencies added with appropriate versions.
The added dependencies enhance CSS processing capabilities with up-to-date versions:
- cssnano for minification
- icss-utils for CSS Modules support
- stylus as an additional CSS preprocessor option
Also applies to: 23-23, 35-35
Line range hint 21-36: Verify CSS processor compatibility and necessity.
The project now includes multiple CSS preprocessors (Stylus, Less, Sass). Consider documenting the use case for each preprocessor to justify maintaining multiple solutions.
Let's check for any examples or configuration files that utilize these processors:
packages/packem/src/rollup/watch.ts (2)
21-29: LGTM: Parameter signature update improves configuration control.
The addition of the useCache parameter makes the caching behavior more explicit and configurable, improving the function's flexibility.
181-182: Document cache disabling conditions.
The useCache parameter is correctly passed to both bundle and type generation watchers. However, consider adding documentation explaining why isolated declarations and CSS loaders require cache disabling.
Let's verify the usage of these features:
Also applies to: 207-208
packages/packem/src/rollup/plugins/css/loaders/postcss/index.ts (2)
18-18: LGTM: Import and variable declaration changes look good.
The uncommented loadConfig import and the new injectorId variable improve the code organization by properly handling configuration loading and caching the injector module path.
Also applies to: 28-28
Line range hint 1-318: Overall assessment: Code is well-structured but has room for improvement.
The implementation is solid and handles CSS processing effectively. The suggested improvements focus on:
- Better documentation
- Enhanced error handling
- More informative warnings
- Code modularization
- Performance optimizations
None of these are critical issues, but implementing them would improve maintainability and robustness.
packages/packem/__tests__/intigration/cli.test.ts (1)
335-335: LGTM! The test case for onSuccess configuration looks good.
The test case properly verifies that the onSuccess command configured in the packem config is executed correctly after a successful build.
packages/packem/README.md (1)
412-418: LGTM! Clear and well-structured plugin ordering documentation.
The plugin ordering is logically organized and follows common bundler practices, making it clear for users to understand how their plugins will be applied during the build process.
packages/packem/src/commands/add.ts (1)
81-81: Ensure correct feature detection when parsing arguments
The code checks if argument.includes("css") to determine if CSS loaders should be added. If argument contains a string where "css" is a substring, this could lead to unintended behavior.
Consider using strict equality or validating against a list of known commands to prevent incorrect feature detection.
-if (argument.includes("css")) {
+if (argument === "css") {Or, if multiple features can be added together, consider parsing argument as an array of features.
| loaders: [lessLoader, postcssLoader, sassLoader, stylusLoader, sourceMapLoader], | ||
| }, | ||
| }, | ||
| loaders: [postcssLoader, stylusLoader, lessLoader, sassLoader, sourceMapLoader,], |
There was a problem hiding this comment.
💡 Codebase verification
The loader order in the configuration file needs to be updated to match the intended processing sequence
The configuration in examples/styles/packem.config.ts has the loaders in the wrong order. According to the core implementation in packages/packem/src/rollup/plugins/css/index.ts, the intended processing sequence is:
- sourcemap
- stylus
- less
- sass
- postcss
The current configuration has postcss first and sourcemap last, which contradicts the sorting order enforced by the sortByNameOrder function.
🔗 Analysis chain
Verify CSS loader order for optimal processing.
The order of CSS loaders can impact the processing pipeline. Please verify that this order is intentional:
- postcss (for PostCSS processing)
- stylus (for Stylus preprocessing)
- less (for Less preprocessing)
- sass (for Sass preprocessing)
- sourcemap (for source map generation)
Consider whether preprocessors (Stylus, Less, Sass) should run before PostCSS for optimal results.
Let me check the actual implementation of the loader ordering logic to understand the intended sequence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any loader order dependencies or documentation
# that might indicate the preferred processing sequence
# Search for loader order documentation or related tests
rg -g '!node_modules' -g '*.{md,ts,js}' -i 'loader.*order|process.*sequence'
Length of output: 523
Script:
#!/bin/bash
# Get the implementation of sortByNameOrder and surrounding context
rg -A 10 "sortByNameOrder" packages/packem/src/rollup/plugins/css/index.ts
Length of output: 786
packages/packem/src/commands/add.ts
Outdated
| import type { Cli } from "@visulima/cerebro"; | ||
| import { readFile, writeFile } from "@visulima/fs"; | ||
| import { resolve } from "@visulima/path"; | ||
| import MagicString from "magic-string"; | ||
|
|
||
| import findPackemFile from "../utils/find-packem-file"; | ||
| import cssLoaderDependencies from "./utils/css-loader-dependencies"; | ||
| import type cssMinifierDependencies from "./utils/css-minifier-dependencies"; |
There was a problem hiding this comment.
Fix incorrect 'import type' for 'cssMinifierDependencies'
The cssMinifierDependencies is imported using import type, but it's being used as a value in the code (e.g., in typeof cssMinifierDependencies). This will cause runtime errors because import type only brings in types, not actual values.
Apply this diff to fix the import:
-import type cssMinifierDependencies from "./utils/css-minifier-dependencies";
+import cssMinifierDependencies from "./utils/css-minifier-dependencies";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type cssMinifierDependencies from "./utils/css-minifier-dependencies"; | |
| import cssMinifierDependencies from "./utils/css-minifier-dependencies"; |
| packagesToInstall.push(cssMinifier); | ||
| } | ||
|
|
There was a problem hiding this comment.
Avoid adding undefined or duplicate packages
When adding the CSS minifier to packagesToInstall, ensure that the variable cssMinifier is defined and not already included.
Add a check to prevent undefined values and duplicate entries.
if (cssMinifier && !packagesToInstall.includes(cssMinifier)) {
packagesToInstall.push(cssMinifier);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| packagesToInstall.push(cssMinifier); | |
| } | |
| if (cssMinifier && !packagesToInstall.includes(cssMinifier)) { | |
| packagesToInstall.push(cssMinifier); | |
| } | |
packages/packem/src/commands/add.ts
Outdated
| s.start("Installing packages"); | ||
| await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true }); | ||
| s.stop("Installed packages"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle installation errors gracefully
When installing packages, errors may occur (e.g., network issues, package not found). Currently, the code does not handle such exceptions.
Wrap the package installation in a try-catch block to handle errors gracefully and inform the user.
try {
loadingSpinner.start("Installing packages");
await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
loadingSpinner.stop("Installed packages");
+} catch (error) {
+ loadingSpinner.stop("Failed to install packages");
+ logger.error(`Package installation failed: ${error.message}`);
+ return;
}Committable suggestion skipped: line range outside the PR's diff.
| cssLoaders.push("sourceMap"); | ||
|
|
||
| for (let loader of cssLoaders) { | ||
| if (loader === "sass-embedded" || loader === "node-sass") { | ||
| loader = "sass"; | ||
| } | ||
|
|
||
| if (packemConfigFormat === "cjs") { | ||
| magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`); | ||
| } else { | ||
| magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Review inclusion of 'sourceMap' in 'cssLoaders'
Adding "sourceMap" to cssLoaders and treating it as a loader may cause issues, as there may not be a corresponding loader module named sourceMapLoader.
Remove "sourceMap" from cssLoaders and handle the source map configuration separately. This prevents potential runtime errors due to importing a non-existent module.
Apply this diff to fix the issue:
-cssLoaders.push("sourceMap");
for (let loader of cssLoaders) {
if (loader === "sass-embedded" || loader === "node-sass") {
loader = "sass";
}
- if (packemConfigFormat === "cjs") {
- magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`);
- } else {
- magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`);
- }
+ const loaderImport = packemConfigFormat === "cjs"
+ ? `const ${loader}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase()}");\n`
+ : `import ${loader}Loader from "@visulima/packem/css/loader/${loader.toLowerCase()}";\n`;
+
+ magic.prepend(loaderImport);
}
+// Handle source map configuration separately if required📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cssLoaders.push("sourceMap"); | |
| for (let loader of cssLoaders) { | |
| if (loader === "sass-embedded" || loader === "node-sass") { | |
| loader = "sass"; | |
| } | |
| if (packemConfigFormat === "cjs") { | |
| magic.prepend(`const ${loader as string}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase() as string}");\n`); | |
| } else { | |
| magic.prepend(`import ${loader as string}Loader from "@visulima/packem/css/loader/${loader.toLowerCase() as string}";\n`); | |
| } | |
| } | |
| for (let loader of cssLoaders) { | |
| if (loader === "sass-embedded" || loader === "node-sass") { | |
| loader = "sass"; | |
| } | |
| const loaderImport = packemConfigFormat === "cjs" | |
| ? `const ${loader}Loader = require("@visulima/packem/css/loader/${loader.toLowerCase()}");\n` | |
| : `import ${loader}Loader from "@visulima/packem/css/loader/${loader.toLowerCase()}";\n`; | |
| magic.prepend(loaderImport); | |
| } | |
| // Handle source map configuration separately if required |
packages/packem/src/commands/init.ts
Outdated
| lightningcss: ["lightningcss"], | ||
| }; | ||
| import cssLoaderDependencies from "./utils/css-loader-dependencies"; | ||
| import type cssMinifierDependencies from "./utils/css-minifier-dependencies"; |
There was a problem hiding this comment.
Incorrect use of 'import type' for value import
At line 9, you're using import type to import cssMinifierDependencies. The import type syntax is intended for importing TypeScript types and interfaces, which are erased at compile time. Since cssMinifierDependencies is used as a value in your code (e.g., when pushing to packagesToInstall), it should be imported without the type keyword.
Apply this diff to fix the issue:
-import type cssMinifierDependencies from "./utils/css-minifier-dependencies";
+import cssMinifierDependencies from "./utils/css-minifier-dependencies";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type cssMinifierDependencies from "./utils/css-minifier-dependencies"; | |
| import cssMinifierDependencies from "./utils/css-minifier-dependencies"; |
packages/packem/src/commands/init.ts
Outdated
| @@ -417,6 +378,12 @@ | |||
|
|
|||
| const extension = hasTypescript ? "ts" : "js"; | |||
|
|
|||
| if (packagesToInstall.length > 0) { | |||
| s.start("Installing packages"); | |||
| await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true }); | |||
There was a problem hiding this comment.
Ensure consistent use of 'options.dir' for package installation
In line 383, you are using process.cwd() as the current working directory when installing packages. However, throughout the script, you reference options.dir for directory operations, which may differ from the current working directory. To ensure that packages are installed in the correct directory, consider using options.dir instead of process.cwd().
Apply this diff to fix the issue:
- await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true });
+ await installPackage(packagesToInstall, { cwd: options.dir, dev: true, silent: true });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await installPackage(packagesToInstall, { cwd: process.cwd(), dev: true, silent: true }); | |
| await installPackage(packagesToInstall, { cwd: options.dir, dev: true, silent: true }); |
…em [1.5.0](https://github.com/visulima/packem/compare/@visulima/packem@1.4.2...@visulima/packem@1.5.0) (2024-11-05) ### Features * added css to packem add command ([#53](#53)) ([dbcba16](dbcba16))
Summary by CodeRabbit
Release Notes
New Features
cssnanofor minification.Bug Fixes
Documentation
Tests
onSuccessfunctionality in CLI commands.Chores