feat: added support for url which imports files as data-URIs or ES #60
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 0 minutes and 49 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 several changes to the 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! 🙏 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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: 10
🧹 Outside diff range and nitpick comments (14)
packages/packem/src/utils/clean-distribution-directories.ts (3)
9-15: Simplify the Set creation and type handling.The type assertion
as unknown as Set<string>is unnecessary since we're creating a new Set from the array.- for (const directory of new Set( - // eslint-disable-next-line @typescript-eslint/require-array-sort-compare - context.options.entries - .map((entry) => entry.outDir) - .filter(Boolean) - .sort() as unknown as Set<string>, - )) { + const directories = [...new Set( + context.options.entries + .map((entry) => entry.outDir) + .filter(Boolean) + )].sort(); + for (const directory of directories) {
16-24: Consider improving directory path validation.The directory path checking logic could be more robust by:
- Normalizing paths before comparison
- Using path manipulation utilities for safer path checks
+ const normalizedDir = directory.endsWith("/") ? directory : `${directory}/`; + const normalizedRootDir = context.options.rootDir.endsWith("/") + ? context.options.rootDir + : `${context.options.rootDir}/`; if ( - directory === context.options.rootDir || + normalizedDir === normalizedRootDir || directory === context.options.sourceDir || - context.options.rootDir.startsWith(directory.endsWith("/") ? directory : `${directory}/`) || + normalizedRootDir.startsWith(normalizedDir) || cleanedDirectories.some((c) => directory.startsWith(c)) ) {
29-31: Consider parallel directory cleaning for better performance.The current implementation cleans directories sequentially. For better performance, consider cleaning them in parallel.
- // eslint-disable-next-line no-await-in-loop - await emptyDir(directory); + const cleaningPromises = directories.map((directory) => emptyDir(directory)); + await Promise.all(cleaningPromises);Note: Only implement this if parallel cleaning is safe for your use case and directories are not nested.
packages/packem/src/utils/remove-old-cache-folders.ts (2)
11-16: Consider optimizing directory filtering.The current implementation reads all entries and then filters directories. Consider combining these operations for better efficiency.
Here's a more efficient approach:
- // eslint-disable-next-line security/detect-non-literal-fs-filename - let cacheDirectories = await readdir(cachePath, { - withFileTypes: true, - }); - - cacheDirectories = cacheDirectories.filter((dirent) => dirent.isDirectory()); + // eslint-disable-next-line security/detect-non-literal-fs-filename + const cacheDirectories = (await readdir(cachePath, { withFileTypes: true })) + .filter((dirent) => dirent.isDirectory());
20-39: Enhance logging and code readability.A few suggestions to improve this section:
- Use template literals for the log message
- Consider simplifying the logging logic
Apply these improvements:
if (!keyStore[dirent.name]) { // eslint-disable-next-line no-await-in-loop await rm(join(cachePath, dirent.name), { force: true, recursive: true, }); - if (hasLogged) { + if (hasLogged) { logger.raw("\n\n"); - } + hasLogged = false; + } logger.info({ - message: "Removing " + dirent.name + " file cache, the cache key is not used anymore.", + message: `Removing ${dirent.name} file cache, the cache key is not used anymore.`, prefix: "file-cache", }); - - hasLogged = false; }packages/packem/__tests__/intigration/url.test.ts (1)
1-14: Fix typo in directory name: "intigration" should be "integration"The directory name contains a spelling error that should be corrected for better maintainability and consistency.
packages/packem/package.json (1)
Line range hint
2-2: Consider bumping minor version instead of patchSince this PR adds new functionality (URL imports as data-URIs), according to semver, it should bump the minor version to "1.6.0" instead of the patch version to "1.5.1".
- "version": "1.5.1", + "version": "1.6.0",packages/packem/src/packem.ts (2)
30-31: Good modularization of utility functions!Moving
cleanDistributionDirectoriesandremoveOldCacheFoldersto separate utility files improves code organization and follows the Single Responsibility Principle.This modularization:
- Improves maintainability by isolating specific functionalities
- Makes the code more testable
- Reduces the complexity of the main file
Also applies to: 40-41
Line range hint
1-824: Well-structured code with robust error handling!The overall implementation demonstrates several good practices:
- Comprehensive error handling with proper enhancement
- Clean process termination handling
- Strong TypeScript typing
- Detailed logging
Consider adding JSDoc comments for the main exported functions to improve documentation.
packages/packem/__tests__/intigration/css.test.ts (4)
Line range hint
1-24: Fix typo in directory nameThe directory name contains a typo:
intigrationshould beintegration.
75-77: Simplify path handling in cpSyncThe
joinoperation is redundant here asfixturePathalready includes the directory path. You can directly use string concatenation with the input path.- cpSync(join(fixturePath, dirname(input[0] as string)), temporaryDirectoryPath, { recursive: true }); + cpSync(`${fixturePath}/${dirname(input[0] as string)}`, temporaryDirectoryPath, { recursive: true });
150-158: Address TODO comment and deprecation warningThere's a TODO comment about changing the
readdirimplementation to use@visulima/fs. Additionally, the code uses a deprecated feature.Would you like me to help implement the
@visulima/fsreaddir solution to address both the TODO and deprecation warning?
Multiple Skipped Tests Found
The shell script detected numerous files with skipped tests, indicating widespread gaps in test coverage. Please review and address these skipped tests to ensure comprehensive testing.
🔗 Analysis chain
Line range hint
590-600: Address skipped and commented testsSeveral test cases are either skipped or commented out:
- The entire stylus test suite is skipped
- Some sass import tests are commented out
- DTS-related tests are commented out
These gaps in test coverage should be addressed to ensure comprehensive testing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other skipped tests in the codebase rg -l "describe\.skip|it\.skip|xdescribe|xit" --type tsLength of output: 4044
packages/packem/src/rollup/plugins/url.ts (1)
170-170: Consider using a stronger hash functionThe code uses the SHA-1 hashing algorithm, which is considered outdated and has known vulnerabilities. While it's used here for file naming and not for security purposes, it's recommended to use a stronger algorithm like SHA-256 to prevent potential hash collisions.
Apply this diff to use SHA-256:
- const hash = crypto.createHash("sha1").update(buffer).digest("hex").slice(0, 16); + const hash = crypto.createHash("sha256").update(buffer).digest("hex").slice(0, 16);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (7)
packages/packem/__fixtures__/url/gif.gifis excluded by!**/*.gifpackages/packem/__fixtures__/url/jpeg.jpegis excluded by!**/*.jpegpackages/packem/__fixtures__/url/jpg.jpgis excluded by!**/*.jpgpackages/packem/__fixtures__/url/png.pngis excluded by!**/*.pngpackages/packem/__fixtures__/url/svg.svgis excluded by!**/*.svgpackages/packem/__tests__/intigration/__snapshots__/url.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
packages/packem/LICENSE.md(1 hunks)packages/packem/__fixtures__/url/gif.js(1 hunks)packages/packem/__fixtures__/url/jpeg.js(1 hunks)packages/packem/__fixtures__/url/jpg.js(1 hunks)packages/packem/__fixtures__/url/png.js(1 hunks)packages/packem/__fixtures__/url/svg.js(1 hunks)packages/packem/__fixtures__/url/webp.js(1 hunks)packages/packem/__tests__/intigration/css.test.ts(3 hunks)packages/packem/__tests__/intigration/url.test.ts(1 hunks)packages/packem/package.json(2 hunks)packages/packem/packem.config.ts(1 hunks)packages/packem/src/constants.ts(1 hunks)packages/packem/src/packem.ts(5 hunks)packages/packem/src/rollup/get-rollup-options.ts(2 hunks)packages/packem/src/rollup/plugins/css/loaders/postcss/url/inline.ts(1 hunks)packages/packem/src/rollup/plugins/url.ts(1 hunks)packages/packem/src/types.ts(3 hunks)packages/packem/src/utils/clean-distribution-directories.ts(1 hunks)packages/packem/src/utils/remove-old-cache-folders.ts(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/packem/fixtures/url/gif.js
- packages/packem/fixtures/url/jpeg.js
- packages/packem/fixtures/url/jpg.js
- packages/packem/fixtures/url/png.js
- packages/packem/fixtures/url/svg.js
- packages/packem/fixtures/url/webp.js
- packages/packem/packem.config.ts
🧰 Additional context used
🪛 Markdownlint
packages/packem/LICENSE.md
77-77: null
Bare URL used
(MD034, no-bare-urls)
🪛 GitHub Check: CodeQL
packages/packem/src/rollup/plugins/url.ts
[failure] 112-117: Incomplete multi-character sanitization
This string may still contain <!--, which may cause an HTML element injection vulnerability.
🔇 Additional comments (14)
packages/packem/src/rollup/plugins/css/loaders/postcss/url/inline.ts (1)
1-1: LGTM! Verify MIME type coverage for all supported file types.
The switch to mime/lite is a good optimization. However, let's ensure it supports all necessary MIME types for the URL plugin.
packages/packem/src/utils/clean-distribution-directories.ts (3)
1-4: LGTM! Clean and focused imports.
The imports are minimal and specific to the requirements.
5-7: LGTM! Well-typed function declaration.
The function signature is clear and properly typed.
35-35: LGTM! Clean export.
The default export is appropriate for this utility function.
packages/packem/src/constants.ts (2)
Line range hint 1-45: Verify handling of removed image formats.
The removal of image format loaders (.gif, .jpeg, .jpg, .png, .svg, .webp) from DEFAULT_LOADERS suggests these will now be handled by the URL plugin.
Let's verify the new handling mechanism:
#!/bin/bash
# Description: Verify that the URL plugin properly handles the removed formats
# and that there are no orphaned image imports.
# Check for the URL plugin implementation
echo "Checking URL plugin implementation..."
rg -A 5 "urlPlugin" packages/packem/src/
# Check for image imports in the codebase
echo "Checking image imports..."
rg -g '*.{ts,tsx,js,jsx}' '\.(gif|jpe?g|png|svg|webp)'9-9: LGTM! Loader changes align with TypeScript conventions.
The change from "ts" to "tsx" loader for .ctsx and .mtsx extensions is correct as these are TypeScript JSX files.
Let's verify the usage of these extensions in the codebase:
Also applies to: 18-18
packages/packem/src/utils/remove-old-cache-folders.ts (1)
1-7: LGTM! Clean imports and well-typed function signature.
The imports are appropriate, and the TypeScript types are properly defined.
packages/packem/src/types.ts (1)
34-34: LGTM! URL plugin configuration properly integrated.
The new URL plugin configuration has been properly integrated into the type system:
- The
UrlOptionstype import follows the established pattern - The optional
urlproperty inRollupBuildOptionsmaintains consistency with other plugin configurations
Also applies to: 110-110
packages/packem/__tests__/intigration/url.test.ts (2)
15-25: LGTM! Well-structured test setup and teardown
Good implementation of test isolation using temporary directories with proper cleanup.
82-313: LGTM! Comprehensive test coverage
The test suite provides excellent coverage with:
- Tests for various file types (PNG, JPG, JPEG, GIF, WEBP, SVG)
- Different scenarios (inlining, copying, size limits)
- Edge cases (nested directories, file naming patterns)
- Configuration options (publicPath, sourceDir, destDir)
packages/packem/package.json (1)
386-387: LGTM: Dependency update looks good
The replacement of mime-types with mime is a good choice as it provides better TypeScript support and is more suitable for handling data-URIs. The version is compatible with the project's Node.js engine requirements.
packages/packem/src/packem.ts (1)
317-323: LGTM! URL configuration looks well-structured.
The URL configuration is properly configured with:
- Appropriate file size limit (14KB) for inlining
- Common image format support
- Content-based hash naming
- Proper exclusion patterns
Let's verify the EXCLUDE_REGEXP pattern matches the URL configuration:
packages/packem/src/rollup/get-rollup-options.ts (2)
54-54: LGTM!
The import statement follows the existing import style and is properly positioned with related plugin imports.
498-499: LGTM!
The URL plugin is correctly configured:
- Follows the existing pattern of conditional plugin addition
- Properly positioned in the plugins array
- Configuration is passed through from the context options
Let's verify the URL plugin configuration:
✅ Verification successful
URL plugin configuration verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for URL plugin configuration and usage patterns
# Check for URL plugin configuration patterns
rg -A 5 'rollup\.url'
# Check for URL plugin imports and usage
ast-grep --pattern 'import { urlPlugin } from "./plugins/url"'
Length of output: 754
Script:
#!/bin/bash
# Description: Verify the positioning and conditional addition of urlPlugin in the plugins array
# Locate the plugins array in get-rollup-options.ts and display surrounding lines for context
rg -A 10 'plugins\s*:\s*\[' packages/packem/src/rollup/get-rollup-options.ts
# Check the placement of urlPlugin within the plugins array
rg 'urlPlugin' packages/packem/src/rollup/get-rollup-options.ts -A 2 -B 2
Length of output: 1632
| export default (file: string, source: Uint8Array): string => { | ||
| const mime = lookup(file) || "application/octet-stream"; | ||
| const mime = liteMime.getType(file) ?? "application/octet-stream"; | ||
| const data = Buffer.from(source).toString("base64"); | ||
|
|
||
| return `data:${mime};base64,${data}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and size limits for robustness.
The function works but could benefit from some safety improvements:
- Add input validation for parameters
- Implement size limits to prevent memory issues
- Consider handling empty or invalid source buffers
Consider applying these improvements:
-export default (file: string, source: Uint8Array): string => {
+const MAX_FILE_SIZE = 1024 * 1024; // 1MB limit for inline files
+
+export default (file: string, source: Uint8Array): string => {
+ if (!file || typeof file !== 'string') {
+ throw new Error('File path must be a non-empty string');
+ }
+
+ if (!source || !(source instanceof Uint8Array)) {
+ throw new Error('Source must be a Uint8Array');
+ }
+
+ if (source.length > MAX_FILE_SIZE) {
+ throw new Error(`File size exceeds maximum limit of ${MAX_FILE_SIZE} bytes`);
+ }
+
const mime = liteMime.getType(file) ?? "application/octet-stream";
const data = Buffer.from(source).toString("base64");
return `data:${mime};base64,${data}`;
};📝 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.
| export default (file: string, source: Uint8Array): string => { | |
| const mime = lookup(file) || "application/octet-stream"; | |
| const mime = liteMime.getType(file) ?? "application/octet-stream"; | |
| const data = Buffer.from(source).toString("base64"); | |
| return `data:${mime};base64,${data}`; | |
| const MAX_FILE_SIZE = 1024 * 1024; // 1MB limit for inline files | |
| export default (file: string, source: Uint8Array): string => { | |
| if (!file || typeof file !== 'string') { | |
| throw new Error('File path must be a non-empty string'); | |
| } | |
| if (!source || !(source instanceof Uint8Array)) { | |
| throw new Error('Source must be a Uint8Array'); | |
| } | |
| if (source.length > MAX_FILE_SIZE) { | |
| throw new Error(`File size exceeds maximum limit of ${MAX_FILE_SIZE} bytes`); | |
| } | |
| const mime = liteMime.getType(file) ?? "application/octet-stream"; | |
| const data = Buffer.from(source).toString("base64"); | |
| return `data:${mime};base64,${data}`; | |
| }; |
| import { relative } from "@visulima/path"; | ||
| import { emptyDir } from "@visulima/fs"; | ||
| import type { BuildContext } from "../types"; | ||
|
|
||
| const cleanDistributionDirectories = async (context: BuildContext): Promise<void> => { | ||
| const cleanedDirectories: string[] = []; | ||
|
|
||
| if (context.options.clean) { | ||
| for (const directory of new Set( | ||
| // eslint-disable-next-line @typescript-eslint/require-array-sort-compare | ||
| context.options.entries | ||
| .map((entry) => entry.outDir) | ||
| .filter(Boolean) | ||
| .sort() as unknown as Set<string>, | ||
| )) { | ||
| if ( | ||
| directory === context.options.rootDir || | ||
| directory === context.options.sourceDir || | ||
| context.options.rootDir.startsWith(directory.endsWith("/") ? directory : `${directory}/`) || | ||
| cleanedDirectories.some((c) => directory.startsWith(c)) | ||
| ) { | ||
| // eslint-disable-next-line no-continue | ||
| continue; | ||
| } | ||
|
|
||
| cleanedDirectories.push(directory); | ||
| context.logger.info(`Cleaning dist directory: \`./${relative(context.options.rootDir, directory)}\``); | ||
|
|
||
| // eslint-disable-next-line no-await-in-loop | ||
| await emptyDir(directory); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| export default cleanDistributionDirectories; |
There was a problem hiding this comment.
💡 Codebase verification
Add tests for cleanDistributionDirectories utility.
No test file clean-distribution-directories.test.ts found. Ensure proper test coverage.
🔗 Analysis chain
Verify test coverage and similar utilities.
Let's ensure this utility is properly tested and there's no duplication in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test file
fd "clean-distribution-directories.test.ts" packages/packem/src
# Look for similar utilities
rg -g "!*.test.ts" -g "!*.d.ts" "emptyDir|cleanDir" packages/packem/src
Length of output: 337
| if (cachePath && (await isAccessible(join(cachePath, "keystore1.json")))) { | ||
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); |
There was a problem hiding this comment.
Fix mismatch in keystore filename.
There's a critical inconsistency in the keystore file naming:
- Line 8 checks for
keystore1.json - Line 9 reads from
keystore.json
This mismatch will cause the function to fail silently as it checks for one file but reads from another.
Apply this fix:
- if (cachePath && (await isAccessible(join(cachePath, "keystore1.json")))) {
- const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json"));
+ if (cachePath && (await isAccessible(join(cachePath, "keystore.json")))) {
+ const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json"));📝 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.
| if (cachePath && (await isAccessible(join(cachePath, "keystore1.json")))) { | |
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); | |
| if (cachePath && (await isAccessible(join(cachePath, "keystore.json")))) { | |
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); |
| const removeOldCacheFolders = async (cachePath: string | undefined, logger: Pail, logged: boolean): Promise<void> => { | ||
| if (cachePath && (await isAccessible(join(cachePath, "keystore1.json")))) { | ||
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); | ||
|
|
||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| let cacheDirectories = await readdir(cachePath, { | ||
| withFileTypes: true, | ||
| }); | ||
|
|
||
| cacheDirectories = cacheDirectories.filter((dirent) => dirent.isDirectory()); | ||
|
|
||
| let hasLogged = logged; | ||
|
|
||
| for (const dirent of cacheDirectories) { | ||
| if (!keyStore[dirent.name]) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| await rm(join(cachePath, dirent.name), { | ||
| force: true, | ||
| recursive: true, | ||
| }); | ||
|
|
||
| if (hasLogged) { | ||
| logger.raw("\n\n"); | ||
| } | ||
|
|
||
| logger.info({ | ||
| message: "Removing " + dirent.name + " file cache, the cache key is not used anymore.", | ||
| prefix: "file-cache", | ||
| }); | ||
|
|
||
| hasLogged = false; | ||
| } | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file operations.
The function should handle potential errors from file operations (readdir, rm) to prevent silent failures and provide better debugging information.
Consider wrapping the operations in try-catch blocks:
const removeOldCacheFolders = async (cachePath: string | undefined, logger: Pail, logged: boolean): Promise<void> => {
- if (cachePath && (await isAccessible(join(cachePath, "keystore.json")))) {
- const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json"));
+ if (!cachePath) {
+ return;
+ }
+
+ try {
+ if (!(await isAccessible(join(cachePath, "keystore.json")))) {
+ return;
+ }
+
+ const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json"));
// eslint-disable-next-line security/detect-non-literal-fs-filename
const cacheDirectories = (await readdir(cachePath, { withFileTypes: true }))
.filter((dirent) => dirent.isDirectory());
let hasLogged = logged;
for (const dirent of cacheDirectories) {
if (!keyStore[dirent.name]) {
// eslint-disable-next-line no-await-in-loop
- await rm(join(cachePath, dirent.name), {
- force: true,
- recursive: true,
- });
+ try {
+ await rm(join(cachePath, dirent.name), {
+ force: true,
+ recursive: true,
+ });
+ } catch (error) {
+ logger.error({
+ message: `Failed to remove cache directory ${dirent.name}: ${error instanceof Error ? error.message : String(error)}`,
+ prefix: "file-cache",
+ });
+ continue;
+ }
if (hasLogged) {
logger.raw("\n\n");
hasLogged = false;
}
logger.info({
message: `Removing ${dirent.name} file cache, the cache key is not used anymore.`,
prefix: "file-cache",
});
}
}
+ } catch (error) {
+ logger.error({
+ message: `Failed to process cache cleanup: ${error instanceof Error ? error.message : String(error)}`,
+ prefix: "file-cache",
+ });
}
};📝 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 removeOldCacheFolders = async (cachePath: string | undefined, logger: Pail, logged: boolean): Promise<void> => { | |
| if (cachePath && (await isAccessible(join(cachePath, "keystore1.json")))) { | |
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename | |
| let cacheDirectories = await readdir(cachePath, { | |
| withFileTypes: true, | |
| }); | |
| cacheDirectories = cacheDirectories.filter((dirent) => dirent.isDirectory()); | |
| let hasLogged = logged; | |
| for (const dirent of cacheDirectories) { | |
| if (!keyStore[dirent.name]) { | |
| // eslint-disable-next-line no-await-in-loop | |
| await rm(join(cachePath, dirent.name), { | |
| force: true, | |
| recursive: true, | |
| }); | |
| if (hasLogged) { | |
| logger.raw("\n\n"); | |
| } | |
| logger.info({ | |
| message: "Removing " + dirent.name + " file cache, the cache key is not used anymore.", | |
| prefix: "file-cache", | |
| }); | |
| hasLogged = false; | |
| } | |
| } | |
| } | |
| }; | |
| const removeOldCacheFolders = async (cachePath: string | undefined, logger: Pail, logged: boolean): Promise<void> => { | |
| if (!cachePath) { | |
| return; | |
| } | |
| try { | |
| if (!(await isAccessible(join(cachePath, "keystore.json")))) { | |
| return; | |
| } | |
| const keyStore: Record<string, string> = await readJson(join(cachePath, "keystore.json")); | |
| // eslint-disable-next-line security/detect-non-literal-fs-filename | |
| const cacheDirectories = (await readdir(cachePath, { withFileTypes: true })) | |
| .filter((dirent) => dirent.isDirectory()); | |
| let hasLogged = logged; | |
| for (const dirent of cacheDirectories) { | |
| if (!keyStore[dirent.name]) { | |
| // eslint-disable-next-line no-await-in-loop | |
| try { | |
| await rm(join(cachePath, dirent.name), { | |
| force: true, | |
| recursive: true, | |
| }); | |
| } catch (error) { | |
| logger.error({ | |
| message: `Failed to remove cache directory ${dirent.name}: ${error instanceof Error ? error.message : String(error)}`, | |
| prefix: "file-cache", | |
| }); | |
| continue; | |
| } | |
| if (hasLogged) { | |
| logger.raw("\n\n"); | |
| hasLogged = false; | |
| } | |
| logger.info({ | |
| message: `Removing ${dirent.name} file cache, the cache key is not used anymore.`, | |
| prefix: "file-cache", | |
| }); | |
| } | |
| } | |
| } catch (error) { | |
| logger.error({ | |
| message: `Failed to process cache cleanup: ${error instanceof Error ? error.message : String(error)}`, | |
| prefix: "file-cache", | |
| }); | |
| } | |
| }; |
| // @TODO: Change this readdir to @visulima/fs readdir | ||
| // eslint-disable-next-line deprecation/deprecation | ||
| .map((dirent) => join(dirent.path, dirent.name)); |
There was a problem hiding this comment.
Address TODO comment and deprecated feature usage
- The TODO comment indicates that
readdirshould be changed to use@visulima/fs. - The code uses the deprecated
dirent.pathproperty.
Consider updating this to use the recommended approach:
- .map((dirent) => join(dirent.path, dirent.name));
+ .map((dirent) => join(distributionPath, dirent.name));Would you like me to help implement the switch to @visulima/fs readdir?
Committable suggestion skipped: line range outside the PR's diff.
| [ | ||
| join(temporaryDirectoryPath, "/dist/png.cjs"), | ||
| join(temporaryDirectoryPath, "/dist/png.mjs"), | ||
| join(temporaryDirectoryPath, "/dist/", temporaryDirectoryPath.replace("/tmp/", ""),"/src/6b71fbe07b498a82.png"), |
There was a problem hiding this comment.
Use path.sep for cross-platform compatibility
The hardcoded path separator /tmp/ might cause issues on Windows platforms.
- join(temporaryDirectoryPath, "/dist/", temporaryDirectoryPath.replace("/tmp/", ""),"/src/6b71fbe07b498a82.png"),
+ const tmpDirPrefix = temporaryDirectoryPath.split(path.sep).slice(0, 2).join(path.sep);
+ join(temporaryDirectoryPath, "dist", temporaryDirectoryPath.replace(tmpDirPrefix + path.sep, ""), "src", "6b71fbe07b498a82.png"),Committable suggestion skipped: line range outside the PR's diff.
| }); | ||
|
|
||
| it("should copy the file according to destDir option", async () => { | ||
| expect.assertions(6); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent assertion count
One test uses expect.assertions(6), while others use expect.assertions(5). Please ensure that this difference is intentional.
🔗 Analysis chain
Verify assertion count consistency
Most tests use expect.assertions(5), but this test uses expect.assertions(6). Please verify if this is intentional or if it should be consistent with other tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check assertion counts in test file
rg -B 2 -A 2 "expect\.assertions\([0-9]+\)" packages/packem/__tests__/intigration/url.test.ts
Length of output: 3714
| @@ -435,6 +434,7 @@ | |||
| "icss-utils": "^5.1.0", | |||
| "less": "^4.2.0", | |||
| "lightningcss": "1.28.1", | |||
| "mime": "4.0.4", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate mime dependency
The mime package is listed in both dependencies and devDependencies. Since it's a runtime dependency needed for data-URI handling, it should only be listed in dependencies.
- "mime": "4.0.4",Committable suggestion skipped: line range outside the PR's diff.
| .replaceAll("\t", " ") | ||
| // eslint-disable-next-line regexp/no-unused-capturing-group | ||
| .replaceAll(/<!--(.*(?=-->))-->/g, "") | ||
| .replaceAll("'", "\\i"), |
There was a problem hiding this comment.
Typo in string replacement: Incorrect escape sequence
The .replaceAll("'", "\\i") appears to have an incorrect escape sequence. It likely intends to escape single quotes by replacing ' with \\'. Consider updating it to properly escape single quotes.
Apply this diff to fix the typo:
-.replaceAll("'", "\\i"),
+.replaceAll("'", "\\'")📝 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.
| .replaceAll("'", "\\i"), | |
| .replaceAll("'", "\\'"), |
| const hash = crypto.createHash("sha1").update(buffer).digest("hex").slice(0, 16); | ||
| const extension = extname(id); | ||
| const name = basename(id, extension); | ||
| const relativeDirectory = sourceDirectory ? relative(sourceDirectory, dirname(id)) : (dirname(id).split("/").pop() ?? ""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cross-platform path compatibility
Using .split("/") to manipulate paths may cause issues on Windows systems where the path separator is \. To ensure cross-platform compatibility, consider using path utilities.
Apply this diff to improve path handling:
- const relativeDirectory = sourceDirectory ? relative(sourceDirectory, dirname(id)) : (dirname(id).split("/").pop() ?? "");
+ const relativeDirectory = sourceDirectory
+ ? relative(sourceDirectory, dirname(id))
+ : basename(dirname(id));Ensure you have imported basename from @visulima/path:
+ import { basename, dirname, extname, join, relative } from "@visulima/path";Committable suggestion skipped: line range outside the PR's diff.
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected] |
…em [1.6.0](https://github.com/visulima/packem/compare/@visulima/[email protected]...@visulima/[email protected]) (2024-11-13) ### Features * added support for url which imports files as data-URIs or ES ([#60](#60)) ([7342aad](7342aad)) ### Miscellaneous Chores * **deps:** update dependency postcss to >=8.4.47 ([#55](#55)) ([7261f2c](7261f2c))
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary by CodeRabbit
Release Notes
New Features
mimedependency to ensure compliance.Bug Fixes
Updates
1.5.1in the package configuration.Chores