fix: updated docs about isolated declarations, moved sourcemapsPlugin… #63
Conversation
|
Thank you for following the naming conventions! 🙏 |
WalkthroughThe changes in this pull request include updates to the Changes
Possibly related PRs
Warning Rate limit exceeded@prisis has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 3 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. 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 (
|
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: 2
🧹 Outside diff range and nitpick comments (4)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
1-1: LGTM! Type annotation improves type safety.The addition of
Record<string, string[]>type annotation enhances type safety and makes the structure of the dependencies mapping explicit.Consider adding a JSDoc comment to document the purpose of this mapping, for example:
+/** + * Maps CSS preprocessors to their required npm dependencies. + * Used for automatic dependency installation when a preprocessor is configured. + */ const cssLoaderDependencies: Record<string, string[]> = {packages/packem/packem.config.ts (1)
Line range hint
6-20: Consider adding configuration documentation.The configuration now includes the
isolatedDeclarationTransformer, but it might be helpful to add a comment explaining its purpose and requirements.Consider adding a documentation comment:
export default defineConfig({ cjsInterop: true, externals: ["stylus", "less", "sass", "node-sass", "postcss", "lightningcss"], fileCache: true, + // Enable isolated declarations support (requires TypeScript >=5.5) isolatedDeclarationTransformer, rollup: {packages/packem/README.md (2)
544-567: Consider enhancing the isolated declarations section.While the section provides good information, it could benefit from:
- Adding specific performance metrics or benchmarks comparing isolated vs. non-isolated declarations
- Including a troubleshooting section for common issues
- Adding examples of when to use each supported transformer
### Isolated declaration types (in TypeScript 5.5) > Generating .d.ts files with the default `rollup-plugin-dts` is slow because the TypeScript compiler must perform costly type inference, but these files streamline type checking by removing unnecessary details, and while shipping raw TypeScript files could simplify workflows, it is impractical due to ecosystem assumptions and performance trade-offs, which isolated declarations aim to address. You need to choose of the supported transformer to use isolated declaration types. - [oxc](https://github.com/oxc-project/oxc) - [@swc/core](https://github.com/swc-project/swc) - [typescript](https://github.com/microsoft/TypeScript) Default is `typescript`. +### Performance Comparison + +Benchmark results comparing isolated vs. non-isolated declarations: +- Build time: X% faster +- Memory usage: Y% lower + +### Choosing a Transformer + +- **typescript**: Best for standard TypeScript projects +- **@swc/core**: Optimized for large-scale projects +- **oxc**: Experimental, offers fastest parsing + +### Troubleshooting + +Common issues and solutions when using isolated declarations: +1. Issue: ... + Solution: ... +2. Issue: ... + Solution: ... ```ts import { defineConfig } from "@visulima/packem/config";
870-883: Enhance transformer documentation with links and comparison.The configuration section could be more helpful by:
- Adding direct links to each transformer's documentation
- Including a comparison table of transformer features
- Providing guidance on choosing the right transformer
#### transformer You choose which one of the three supported transformer to use. - [esbuild](https://github.com/evanw/esbuild) - [@swc/core](https://github.com/swc-project/swc) - [sucrase](https://github.com/alangpierce/sucrase) +### Transformer Comparison + +| Feature | esbuild | @swc/core | sucrase | +|---------|---------|-----------|----------| +| Speed | ⭐⭐⭐ | ⭐⭐ | ⭐ | +| Config | Simple | Advanced | Minimal | +| Size | Large | Medium | Small | + +### When to use each transformer: + +- **esbuild**: Best for production builds +- **@swc/core**: Ideal for development +- **sucrase**: Perfect for simple transformations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/packem/README.md(3 hunks)packages/packem/packem.config.ts(1 hunks)packages/packem/src/commands/utils/css-loader-dependencies.ts(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(3 hunks)
🔇 Additional comments (4)
packages/packem/src/commands/utils/css-loader-dependencies.ts (1)
Line range hint 1-19: Verify package validity and completeness
Let's ensure all packages exist and check for any missing preprocessors or dependencies.
✅ Verification successful
Based on the verification results, I can now provide a final response:
All CSS preprocessor packages are valid and well-covered
All listed packages in cssLoaderDependencies exist on npm registry and the codebase shows comprehensive coverage of CSS preprocessing needs:
- All core preprocessors are included: Less, Sass (with multiple implementations), Stylus, PostCSS
- The PostCSS dependencies list is complete with all necessary modules for CSS modules functionality
- The codebase usage patterns confirm the completeness of the dependencies through extensive imports and implementations
- LightningCSS is correctly included as a modern CSS processor option
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify npm packages exist and check for common CSS preprocessors
# Check if all listed packages exist on npm
echo "Verifying package existence..."
packages=(
"less"
"lightningcss"
"node-sass"
"postcss"
"postcss-load-config"
"postcss-modules-extract-imports"
"postcss-modules-local-by-default"
"postcss-modules-scope"
"postcss-modules-values"
"postcss-value-parser"
"icss-utils"
"sass"
"sass-embedded"
"stylus"
)
for pkg in "${packages[@]}"; do
if curl --silent "https://registry.npmjs.org/$pkg" | jq -e '.error' > /dev/null; then
echo "❌ Package not found: $pkg"
else
echo "✅ Package exists: $pkg"
fi
done
# Search for any CSS preprocessor imports in the codebase that might need dependencies
echo -e "\nSearching for potential missing preprocessors..."
rg -g '!node_modules' -g '*.{js,ts,json}' -i '(import|require).*\b(css|style|sass|less|stylus|postcss)\b'
Length of output: 23765
packages/packem/packem.config.ts (1)
3-3: Verify the transformer import usage across the codebase.
Let's ensure this new transformer is properly imported and used consistently.
✅ Verification successful
Let me check the actual usage and implementation to verify if the transformer is properly integrated.
Based on the gathered information, I can now provide a final response:
The transformer import is properly integrated and documented
The isolated declarations transformer is:
- Correctly imported and used in the build pipeline
- Well integrated with proper type definitions in
types.ts - Properly documented in README.md with version requirements (TS 5.5+)
- Used consistently across build, watch, and type generation processes
- Has proper configuration options and examples
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other usages or references to the isolated declarations transformer
# Search for other potential uses of the transformer
rg -l "isolatedDeclarationTransformer|isolated-declarations-typescript-transformer"
# Check if the transformer is documented in any README or documentation files
rg -l "isolated declaration" -g "*.md"
Length of output: 755
Script:
#!/bin/bash
# Check the transformer implementation and its usage in build files
rg -A 5 "isolatedDeclarationTransformer" packages/packem/src/rollup/
# Check documentation
cat packages/packem/README.md | grep -A 10 "isolated declaration"
# Check configuration types
ast-grep --pattern 'interface $_ {
$$$
isolatedDeclarations?: $_
$$$
}'
Length of output: 7969
packages/packem/README.md (1)
56-56: LGTM! Clear feature list addition.
The new feature is well-documented with the correct TypeScript version requirement.
packages/packem/src/rollup/get-rollup-options.ts (1)
530-540: 🛠️ Refactor suggestion
Verify TypeScript version compatibility for isolated declarations.
The isolated declarations feature requires TypeScript 5.5 or higher. However, there's no version check in the code to ensure compatibility.
Let's verify the TypeScript version requirements:
Add a version check before enabling isolated declarations:
if (context.options.declaration &&
context.options.rollup.isolatedDeclarations &&
context.options.isolatedDeclarationTransformer) {
// Add version check
const tsVersion = getTsVersion(); // implement this helper
if (semver.lt(tsVersion, '5.5.0')) {
context.logger.warn('Isolated declarations require TypeScript >= 5.5');
return;
}
isolatedDeclarationsPlugin(
join(context.options.rootDir, context.options.sourceDir),
context.options.isolatedDeclarationTransformer,
context.options.declaration,
Boolean(context.options.rollup.cjsInterop),
context.options.rollup.isolatedDeclarations,
)
}
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
examples/isolated-declarations-swc/packem.config.ts (1)
12-12: Consider adding a code comment explaining the version requirement.Since this is an example project demonstrating isolated declarations, it would be helpful to add a comment explaining why TypeScript >=5.5 is required.
rollup: { node10Compatibility: { + // TypeScript >=5.5 is required for isolated declarations support typeScriptVersion: ">=5.5", writeToPackageJson: true, }, },examples/isolated-declarations-typescript/package.json (1)
22-26: Document TypeScript version requirementSince this example specifically demonstrates isolated declarations which requires TypeScript >=5.5, consider adding a note in the package's README or adding a engines.typescript field to explicitly document this requirement.
Add the following to package.json:
{ "name": "examples_packem_isolated_declarations_typescript", + "engines": { + "typescript": ">=5.5" + }, ... }examples/isolated-declarations-oxc/package.json (1)
20-20: Consider updating the defaulttypesfield for consistency.The default
typesfield points to./dist/index.d.ts, whiletypesVersionspoints to./dist/index.d.cts. Consider updating the defaulttypesfield to match the CJS type definition for consistency with the package's dual-format setup.- "types": "./dist/index.d.ts", + "types": "./dist/index.d.cts",Also applies to: 22-26
packages/packem/src/commands/add.ts (3)
132-132: Avoid unnecessary type assertionsAt line 132, there is a type assertion using
as string[]. Unnecessary type assertions can bypass TypeScript's type checking and potentially introduce runtime errors. Consider refining the types to avoid these assertions, which will improve type safety and code clarity.You might adjust the code as follows to let TypeScript infer the correct type:
-packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies] as string[]); +packagesToInstall.push(...cssLoaderDependencies[loader]);Ensure that
cssLoaderDependenciesis properly typed so that TypeScript can infer the return type without assertions.
Line range hint
152-163: Ensure consistent handling of CSS minifier importsIn the block starting at line 152, when importing the CSS minifier, ensure that the logic for checking
packemConfigFormatis consistent with how other imports are handled. This maintains consistency across the codebase and reduces potential import issues.Consider aligning the import statements as follows:
if (packemConfigFormat === "cjs") { magic.prepend( - `const ${cssMinifier as string}Minifier = require("@visulima/packem/css/minifier/${cssMinifier.toLowerCase() as string}");\n`, + `const ${cssMinifier}Minifier = require("@visulima/packem/css/minifier/${cssMinifier}");\n`, ); } else { magic.prepend( - `import ${cssMinifier as string}Minifier from "@visulima/packem/css/minifier/${cssMinifier.toLowerCase() as string}";\n`, + `import ${cssMinifier}Minifier from "@visulima/packem/css/minifier/${cssMinifier}";\n`, ); }This assumes that the directory naming matches the casing of
${cssMinifier}.
Line range hint
195-199: Handle optional chaining to prevent runtime errorsThe replacement logic starting at line 195 might lead to issues if
transformerPlaceholderis undefined or does not match the expected value. Consider using optional chaining or verifying thattransformerPlaceholderis defined before using it.Ensure that
transformerPlaceholderexists and adjust the replacement logic to handle cases where it might not be found.- magic.replace( + if (transformerPlaceholder) { + magic.replace( transformerPlaceholder, ` transformer,\n rollup: {\n css: {${cssMinifier ? `\n minifier: ${cssMinifier}Minifier,` : ""}\n loaders: [${stringCssLoaders}],\n },\n },`, ); + } else { + // Handle the case where transformerPlaceholder is not found + }This prevents potential runtime errors due to undefined variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
examples/isolated-declarations-oxc/package.json(1 hunks)examples/isolated-declarations-oxc/packem.config.ts(1 hunks)examples/isolated-declarations-swc/package.json(1 hunks)examples/isolated-declarations-swc/packem.config.ts(1 hunks)examples/isolated-declarations-typescript/package.json(1 hunks)examples/isolated-declarations-typescript/packem.config.ts(1 hunks)packages/packem/package.json(1 hunks)packages/packem/packem.config.ts(1 hunks)packages/packem/src/commands/add.ts(2 hunks)packages/packem/src/rollup/get-rollup-options.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/packem/packem.config.ts
- packages/packem/src/rollup/get-rollup-options.ts
🔇 Additional comments (9)
examples/isolated-declarations-oxc/packem.config.ts (2)
12-12: LGTM: TypeScript version constraint aligns with isolated declarations feature
The addition of typeScriptVersion: ">=5.5" correctly specifies the minimum TypeScript version required for isolated declarations support.
12-12: Verify version constraint consistency across example configs
Let's ensure this version constraint is consistent across all isolated declaration example configurations.
✅ Verification successful
TypeScript version constraints are consistent across configs ✅
All packem configuration files in the examples directory specify the same TypeScript version constraint of ">=5.5", showing consistent version requirements across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check TypeScript version constraints across all example configs
# Expected: All isolated-declarations examples should specify the same version constraint
# Search for typeScriptVersion in all packem.config.ts files under examples/isolated-declarations-*
fd 'packem\.config\.ts$' examples/isolated-declarations- --exec grep -l "typeScriptVersion"
# Show the actual version constraints
fd 'packem\.config\.ts$' examples/isolated-declarations- --exec grep -A 1 "typeScriptVersion"
Length of output: 423
Script:
#!/bin/bash
# Let's try a different approach to find all packem config files
fd 'packem\.config\.ts$' examples --exec grep -A 1 "typeScriptVersion"
Length of output: 304
examples/isolated-declarations-swc/packem.config.ts (1)
12-12: LGTM! Verify version constraint propagation.
The TypeScript version constraint of ">=5.5" is correct for isolated declarations support. Since writeToPackageJson is enabled, this will be reflected in package.json.
Let's verify the version constraint is consistently set across related files:
✅ Verification successful
TypeScript version constraint is correctly aligned across isolated declaration examples
The version constraint ">=5.5" is consistently set in all three isolated declaration example configurations:
- examples/isolated-declarations-typescript/packem.config.ts
- examples/isolated-declarations-oxc/packem.config.ts
- examples/isolated-declarations-swc/packem.config.ts
All three configs have the same settings:
rollup: {
node10Compatibility: {
typeScriptVersion: ">=5.5",
writeToPackageJson: true,
},
}This aligns with the package.json TypeScript dependencies which show compatibility with TypeScript 5.6.x.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for TypeScript version constraints across config files and package.json
# Expected: All related files should specify >=5.5 for isolated declarations
# Check all packem config files for typeScriptVersion
echo "Checking packem config files:"
rg -l "typeScriptVersion.*5\.5" "examples/**/packem.config.ts"
# Check if documented in README
echo -e "\nChecking README for version requirement:"
rg "TypeScript.*5\.5" "README.md"
Length of output: 333
Script:
#!/bin/bash
# Let's try a different approach to find the config files and version constraints
# First, let's find all packem config files
echo "Finding all packem config files:"
fd "packem.config.ts"
# Then check their content for TypeScript version
echo -e "\nChecking TypeScript version in configs:"
fd "packem.config.ts" --exec cat {}
# Also check package.json for TypeScript version requirements
echo -e "\nChecking package.json for TypeScript version:"
fd "package.json" --exec cat {} | grep -i typescript
Length of output: 14867
examples/isolated-declarations-typescript/packem.config.ts (1)
12-12: LGTM! Version constraint aligns with isolated declarations feature requirements.
The TypeScript version constraint of ">=5.5" is correctly specified for the isolated declarations feature.
Let's verify the consistency of this version constraint across the codebase:
✅ Verification successful
Let me verify the content of these files to ensure the version constraints are consistent.
TypeScript version constraint is consistently implemented across the codebase
The version requirement typeScriptVersion: ">=5.5" is correctly and consistently specified in:
- All isolated declarations example configurations (typescript, swc, oxc)
- Main package configuration
- Properly documented in the README.md with context about isolated declarations feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for TypeScript version constraints across the codebase
# Expected: All related files should specify TypeScript >=5.5 for isolated declarations
# Check package.json files for TypeScript version specifications
echo "Checking package.json files for TypeScript version constraints:"
fd package.json --exec rg -l "typescript.*5\.5"
# Check config files for TypeScript version constraints
echo "\nChecking config files for TypeScript version constraints:"
rg -l "typeScriptVersion.*5\.5"
# Check documentation for TypeScript version mentions
echo "\nChecking documentation for TypeScript version requirements:"
rg -l "TypeScript.*5\.5" -g "*.md"
Length of output: 741
Script:
#!/bin/bash
# Check the content of config files and README
echo "Content of packem.config.ts files:"
rg "typeScriptVersion.*5\.5" -A 2 -B 2
echo "\nContent of README.md TypeScript version mentions:"
rg "TypeScript.*5\.5" -A 2 -B 2 packages/packem/README.md
Length of output: 2268
examples/isolated-declarations-typescript/package.json (1)
22-26: Verify TypeScript declaration file configuration
The typesVersions configuration points to .d.cts file for TypeScript >=5.5, which is correct for CommonJS declaration files. However, since the package is of type "module", you might want to consider the following:
- The exports field correctly handles both
.d.mtsand.d.cts - The typesVersions field could potentially include both formats for better compatibility
✅ Verification successful
TypeScript declaration configuration is correct for the package type
The current configuration is actually correct and well-structured:
- The
exportsfield properly handles both ESM and CJS scenarios with their respective declaration files (.d.mtsand.d.cts) - Since this is a type="module" package, the
typesVersionscorrectly points to.d.ctsfor CommonJS consumers using TypeScript >=5.5 - The setup aligns with the
isolatedDeclarations: truein tsconfig.json, which requires TypeScript >=5.5
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence and format of declaration files
# Check if both .d.mts and .d.cts files are generated
fd -e d.mts -e d.cts . examples/isolated-declarations-typescript/dist
# Check if any other examples have similar configuration
rg -l "typesVersions.*>=5.5" examples/
Length of output: 246
Script:
#!/bin/bash
# Let's verify the package configuration and file structure
# Check if the directory exists and list its contents
ls -la examples/isolated-declarations-typescript/
# Check the package.json content to understand the full configuration
cat examples/isolated-declarations-typescript/package.json
# Look for any build scripts or configuration that generates declaration files
rg -A 5 "build" examples/isolated-declarations-typescript/package.json
# Check for tsconfig.json to understand the TypeScript configuration
cat examples/isolated-declarations-typescript/tsconfig.json
Length of output: 2094
examples/isolated-declarations-swc/package.json (1)
22-24: LGTM: TypeScript version constraint is appropriate.
The version constraint >=5.5 correctly aligns with TypeScript's isolated declarations feature introduction.
examples/isolated-declarations-oxc/package.json (1)
22-26: LGTM: TypeScript version constraint aligns with isolated declarations feature.
The version constraint >=5.5 correctly specifies the minimum TypeScript version required for isolated declarations support.
packages/packem/package.json (1)
315-369: Verify TypeScript version compatibility.
The new type definitions for TypeScript 5.5 are added while maintaining support for TypeScript 4.5 and 5.0 as specified in the peerDependencies. This ensures backward compatibility.
Let's verify the TypeScript version constraints:
#!/bin/bash
# Description: Check TypeScript version constraints
# Extract TypeScript version from peerDependencies
TS_VERSION=$(jq -r '.peerDependencies.typescript' package.json)
echo "TypeScript version constraint: $TS_VERSION"
# Verify if the version range includes 5.5
npx semver -r "$TS_VERSION" 5.5.0
if [ $? -eq 0 ]; then
echo "Version range includes TypeScript 5.5"
else
echo "Warning: Version range may not include TypeScript 5.5"
fiAlso applies to: 577-578
packages/packem/src/commands/add.ts (1)
Line range hint 232-235: Verify that all new dependencies are correctly installed
After modifying the list of packages to install, ensure that all required dependencies are correctly installed and added to package.json. This includes any new loaders or minifiers selected by the user.
Run the following script to verify that all dependencies are installed:
This script checks for the presence of the expected packages.
✅ Verification successful
Let me gather more information about the package dependencies and installation process.
Let me check the CSS loader dependencies and installation process.
Based on the review of the code and dependencies, I can now provide a final response:
All CSS-related dependencies are correctly configured and installed
The implementation correctly handles the installation of CSS dependencies through:
- Proper dependency definitions in
cssLoaderDependencies - Correct peer dependency specifications in
package.json - Automated installation via
@antfu/install-pkgduring theaddcommand execution - User-driven selection of loaders and minifiers with appropriate dependency resolution
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required packages are installed and listed in package.json.
cd "$(dirname "$0")"
# List of packages to verify
packages=(
"@visulima/packem/css/loader/*"
"@visulima/packem/css/minifier/*"
# Add other expected packages here
)
for package in "${packages[@]}"; do
if ! npm ls "$package" >/dev/null 2>&1; then
echo "Package $package is not installed."
fi
done
Length of output: 533
Script:
#!/bin/bash
# Check package.json files for dependencies
echo "=== Checking package.json files ==="
fd package.json --exec cat {}
echo -e "\n=== Checking add command implementation ==="
# Check the implementation of the add command
cat packages/packem/src/commands/add.ts
Length of output: 147762
Script:
#!/bin/bash
# Check CSS loader dependencies and installation process
echo "=== Checking CSS loader dependencies ==="
cat packages/packem/src/commands/utils/css-loader-dependencies.ts
echo -e "\n=== Checking package.json for CSS-related dependencies ==="
cat packages/packem/package.json | grep -A 20 '"peerDependencies":'
Length of output: 1686
| ">=5.5": { | ||
| ".": [ | ||
| "./dist/index.d.ts" | ||
| "./dist/index.d.cts" | ||
| ] | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Type declarations configuration needs adjustment
The package.json configuration has inconsistencies in its type declarations:
- The
typesfield points to "./dist/index.d.ts" but there's no corresponding file in the project - The
exportsfield correctly specifies dual-format type declarations:- ESM: "./dist/index.d.mts"
- CJS: "./dist/index.d.cts"
- The
typesVersionsonly includes CJS declarations but misses ESM declarations for TypeScript >=5.5
Recommendations:
- Remove the
typesfield since dual-format declarations are handled viaexports - Update
typesVersionsto include both formats:
"typesVersions": {
">=5.5": {
".": [
"./dist/index.d.mts",
"./dist/index.d.cts"
]
}
}🔗 Analysis chain
Verify type declaration paths consistency.
While the CJS type declaration path is correctly updated, please verify:
- The
typesfield still points to "./dist/index.d.ts" - ESM type declarations might need similar versioning constraints
Let me gather more specific information about the package.json file in question.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check type declaration paths consistency across the project
# Check if there are any .d.ts files that should be .d.cts or .d.mts
fd -e d.ts -x echo "Found .d.ts file: {}"
# Check if other example projects have consistent typesVersions configuration
rg -l "typesVersions" "examples/**/package.json" | xargs cat
Length of output: 802
Script:
#!/bin/bash
# Check the complete package.json content to verify types field and ESM declarations
cat examples/isolated-declarations-swc/package.json
# Check if there are any .d.cts or .d.mts files in the project
fd -e d.cts -e d.mts -x echo "Found {}"
Length of output: 1099
prisis
force-pushed
the
update-isolated-docs
branch
from
60a3985
to
405a98b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (8)
packages/packem/src/commands/add.ts (4)
Line range hint
89-131: Consider extracting loader selection logic into separate functionsThe loader selection logic is quite complex and would benefit from being broken down into smaller, more focused functions for better maintainability and testability.
Consider refactoring like this:
+ const selectMainCssLoader = async () => { + return select({ + message: "Pick a css loader", + options: [ + { label: "PostCSS", value: "postcss" }, + { hint: "experimental", label: "Lightning CSS", value: "lightningcss" }, + ], + }) as Promise<keyof typeof cssLoaderDependencies>; + }; + const selectExtraLoaders = async () => { + return multiselect({ + message: "Pick extra loaders", + options: [ + { label: "Sass", value: "sass" }, + { label: "Stylus", value: "stylus" }, + { label: "Less", value: "less" }, + ], + required: false, + }) as Promise<(keyof typeof cssLoaderDependencies)[]>; + }; + const handleSassVariant = async (extraLoaders: (keyof typeof cssLoaderDependencies)[]) => { + if (!extraLoaders.includes("sass")) return extraLoaders; + + const sassLoader = await select({ + message: "Pick a sass loader", + options: [ + { hint: "recommended", label: "Sass embedded", value: "sass-embedded" }, + { label: "Sass", value: "sass" }, + { hint: "legacy", label: "Node Sass", value: "node-sass" }, + ], + }); + + if (sassLoader === "sass") return extraLoaders; + + return [ + ...extraLoaders.filter((loader) => loader !== "sass"), + sassLoader as keyof typeof cssLoaderDependencies + ]; + }; const cssLoaders: (keyof typeof cssLoaderDependencies | "sourceMap")[] = []; - const mainCssLoader = (await select({...})) as keyof typeof cssLoaderDependencies; + const mainCssLoader = await selectMainCssLoader(); cssLoaders.push(mainCssLoader); - let extraCssLoaders = (await multiselect({...})) as (keyof typeof cssLoaderDependencies)[]; + let extraCssLoaders = await selectExtraLoaders(); - if (extraCssLoaders.includes("sass")) { - const sassLoader = await select({...}); - if (sassLoader !== "sass") { - extraCssLoaders = extraCssLoaders.filter((loader) => loader !== "sass"); - extraCssLoaders.push(sassLoader as keyof typeof cssLoaderDependencies); - } - } + extraCssLoaders = await handleSassVariant(extraCssLoaders); cssLoaders.push(...extraCssLoaders);
132-132: Improve type safety by avoiding type assertionThe type assertion in the array spread operation can be avoided by properly typing the
cssLoaderDependenciesobject.Consider this approach:
- packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies] as string[]); + type LoaderKey = keyof typeof cssLoaderDependencies; + const dependencies = cssLoaderDependencies[loader as LoaderKey]; + if (dependencies) { + packagesToInstall.push(...dependencies); + }
Line range hint
134-196: Consider using a template-based approach for config generationThe current string manipulation approach for generating the config is error-prone and hard to maintain. Consider using a template-based approach or a structured configuration builder.
Here's a suggested approach:
interface CssConfig { minifier?: string; loaders: string[]; } const generateCssConfig = (cssLoaders: string[], minifier?: string): string => { const config: CssConfig = { loaders: cssLoaders.map(loader => loader === "sass-embedded" || loader === "node-sass" ? "sass" : loader ).map(loader => `${loader}Loader`), }; if (minifier) { config.minifier = `${minifier}Minifier`; } return `rollup: { css: ${JSON.stringify(config, null, 2).replace(/"/g, '')}, },`; };This approach would:
- Make the config structure more explicit
- Reduce the risk of string manipulation errors
- Make it easier to maintain and modify the config structure
Line range hint
26-26: Consider breaking down the execute function to reduce complexityThe execute function has high cognitive complexity (as noted by the eslint-disable comment). Consider breaking it down into separate handlers for each feature type.
Example structure:
const handleTypedocFeature = async (config: MagicString, format: string, rootDirectory: string) => { // TypeDoc-specific logic }; const handleCssFeature = async (config: MagicString, format: string, rootDirectory: string) => { // CSS-specific logic }; const execute = async ({ argument, logger, options }): Promise<void> => { // Setup code... if (argument.includes("typedoc")) { await handleTypedocFeature(magic, packemConfigFormat, rootDirectory); } if (argument.includes("css")) { await handleCssFeature(magic, packemConfigFormat, rootDirectory); } // Cleanup code... };This would:
- Improve maintainability
- Make the code easier to test
- Make it easier to add new features in the future
packages/packem/src/commands/init.ts (2)
250-250: Consider improving type safety without explicit castingThe explicit type casting to
string[]suggests potential type inference issues withcssLoaderDependencies. Consider improving the type definition ofcssLoaderDependenciesinstead.- packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies] as string[]); + packagesToInstall.push(...cssLoaderDependencies[loader as keyof typeof cssLoaderDependencies]);This would require updating the type definition in
css-loader-dependencies.tsto ensure proper type inference:type CssLoaderDependencies = Record<string, readonly string[]>;
Line range hint
1-479: Consider refactoring to reduce complexity and improve maintainabilityThe initialization command has high cognitive complexity and handles multiple concerns. Consider breaking it down into smaller, focused functions:
- Package management (detecting/installing dependencies)
- TypeScript configuration
- CSS configuration
- Config file generation
This would improve:
- Testability
- Maintainability
- Code reuse
- Error handling
Example structure:
class PackemInitializer { private readonly rootDir: string; private readonly packageJson: PackageJson; async detectExistingDependencies(): Promise<string[]> { // Extract existing deps detection } async configureTypeScript(): Promise<TypeScriptConfig> { // Extract TypeScript setup } async configureCssProcessing(): Promise<CssConfig> { // Extract CSS setup } async generateConfigFile(config: PackemConfig): Promise<void> { // Extract config generation } }packages/packem/README.md (2)
544-555: Consider enhancing the isolated declarations documentation.While the section provides good information about the feature, consider adding:
- A brief example showing the performance benefits
- A note about compatibility with existing TypeScript projects
- Clear prerequisites for using this feature
The explanation of why this feature is beneficial is well-written, but more practical guidance would help users adopt it effectively.
870-883: Consider restructuring the configuration section.The configuration section could be improved by:
- Adding a brief introduction about when to use each transformer
- Including configuration examples for each transformer option
- Linking to the respective transformer documentation
This would help users make informed decisions about which transformer to use.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
examples/isolated-declarations-oxc/package.json(1 hunks)examples/isolated-declarations-oxc/packem.config.ts(1 hunks)examples/isolated-declarations-swc/package.json(1 hunks)examples/isolated-declarations-swc/packem.config.ts(1 hunks)examples/isolated-declarations-typescript/package.json(1 hunks)examples/isolated-declarations-typescript/packem.config.ts(1 hunks)packages/packem/README.md(3 hunks)packages/packem/__tests__/intigration/url.test.ts(1 hunks)packages/packem/package.json(1 hunks)packages/packem/packem.config.ts(1 hunks)packages/packem/src/commands/add.ts(2 hunks)packages/packem/src/commands/init.ts(1 hunks)packages/packem/src/commands/utils/css-loader-dependencies.ts(1 hunks)packages/packem/src/rollup/get-rollup-options.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- examples/isolated-declarations-oxc/package.json
- examples/isolated-declarations-oxc/packem.config.ts
- examples/isolated-declarations-swc/package.json
- examples/isolated-declarations-swc/packem.config.ts
- examples/isolated-declarations-typescript/package.json
- examples/isolated-declarations-typescript/packem.config.ts
- packages/packem/package.json
- packages/packem/packem.config.ts
- packages/packem/src/commands/utils/css-loader-dependencies.ts
- packages/packem/src/rollup/get-rollup-options.ts
🔇 Additional comments (2)
packages/packem/README.md (2)
56-56: LGTM: Feature list update is accurate and well-formatted.
The new feature "Supports isolated declaration types (experimental) (Typescript version 5.5 or higher)" is correctly added to the feature list with the appropriate version requirement.
556-566: LGTM: Configuration example is clear and well-structured.
The code example effectively demonstrates:
- How to import the necessary components
- How to configure the transformer
- The correct syntax for the configuration
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/packem/__tests__/intigration/url.test.ts (1)
Line range hint
1-1: Fix typo in directory nameThe directory name contains a typo: "intigration" should be "integration". This could affect test discovery and imports. Consider renaming the directory:
mv packages/packem/__tests__/intigration packages/packem/__tests__/integrationRemember to update any references to this path in other files.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
examples/isolated-declarations-oxc/package.json(1 hunks)examples/isolated-declarations-swc/package.json(1 hunks)examples/isolated-declarations-typescript/package.json(1 hunks)packages/packem/__tests__/intigration/url.test.ts(4 hunks)packages/packem/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/isolated-declarations-swc/package.json
- examples/isolated-declarations-typescript/package.json
- packages/packem/package.json
🔇 Additional comments (2)
examples/isolated-declarations-oxc/package.json (2)
Line range hint 22-26: LGTM: TypeScript version constraint is appropriate
The >=5.5 constraint aligns with the isolated declarations feature requirement and matches the installed TypeScript version (^5.6.3).
Line range hint 6-15: Verify dual package hazard handling
The exports configuration correctly handles dual package hazards by providing separate paths for ESM (.mjs/.d.mts) and CJS (.cjs/.d.cts) consumers. Let's verify this setup across the codebase:
✅ Verification successful
Dual package hazard handling is consistently implemented across the codebase
The verification confirms proper dual package hazard handling:
- Multiple packages in both
examples/andpackages/directories consistently implement the dual package pattern using.mts/.ctsextensions - The type definitions are consistently declared in package.json files with proper extensions
- The pattern matches the reviewed file's implementation, showing standardized handling across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent dual package hazard handling across example projects
# Check for similar export configurations in other example projects
echo "Checking export configurations in other example projects:"
fd -e json -x jq -r '.exports | select(.) | keys[]' {}
# Verify the existence of corresponding type definition files
echo "Verifying type definition files:"
fd -g "*.d.mts" -g "*.d.cts" -x echo "Found type definition: {}"
# Check for potential mismatches in type extensions
echo "Checking for type extension consistency:"
rg -g "*.json" '"types":\s*"[^"]+\.d\.(mts|cts)"'
Length of output: 6882
| const pngPath = join(basename(temporaryDirectoryPath), "/src/6b71fbe07b498a82.png"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use path.join for safer path concatenation
While using basename improves cross-platform compatibility, the string concatenation with "/" could still cause issues on Windows. Consider using path.join for the entire path:
-const pngPath = join(basename(temporaryDirectoryPath), "/src/6b71fbe07b498a82.png");
+const pngPath = join(basename(temporaryDirectoryPath), "src", "6b71fbe07b498a82.png");This ensures consistent path separators across all platforms.
📝 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 pngPath = join(basename(temporaryDirectoryPath), "/src/6b71fbe07b498a82.png"); | |
| const pngPath = join(basename(temporaryDirectoryPath), "src", "6b71fbe07b498a82.png"); |
…em [1.7.1](https://github.com/visulima/packem/compare/@visulima/packem@1.7.0...@visulima/packem@1.7.1) (2024-11-17) ### Bug Fixes * updated docs about isolated declarations, moved sourcemapsPlugin… ([#63](#63)) ([d64d022](d64d022))
… to a different position
Summary by CodeRabbit
New Features
Bug Fixes
cssLoaderDependencies, clarifying its structure.Documentation
package.jsonfiles across examples to specify TypeScript version compatibility.Chores