Conversation
…set the `duc` dependency version for publishing.
Jorgedanisc
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
Pull request overview
This PR implements automated dependency management for Rust crates during the publishing process and adds Brotli compression for WASM and JavaScript assets in the ducsvg package. The main change introduces a script that converts local path-based duc dependencies to crates.io versions during semantic-release, enabling proper publication of duc2pdf to crates.io. Additionally, it adds build-time compression to optimize bundle sizes.
Key changes:
- New Node.js script for updating Cargo.toml duc dependencies from local paths to crates.io versions during publishing
- Brotli compression plugin added to ducsvg Vite configuration for WASM and JS assets
- Removed unused workspace.dependencies section from root Cargo.toml
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/cargo-set-duc-dep-version.js | New script to replace path-based duc dependencies with crates.io versions in Cargo.toml files |
| packages/ducsvg/vite.config.mts | Added vite-plugin-compression for Brotli compression and new external patterns for WASM/pdf2svg |
| packages/ducsvg/package.json | Added vite-plugin-compression dependency |
| packages/ducpdf/src/duc2pdf/release.config.cjs | Integrated cargo-set-duc-dep-version script into prepare step with hardcoded version "2" |
| packages/ducpdf/src/duc2pdf/Cargo.toml | Added comment explaining local vs publish dependency format |
| packages/ducpdf/src/duc2pdf/README.md | Added trailing newline |
| Cargo.toml | Removed unused workspace.dependencies section |
| bun.lock | Updated lockfile with new vite-plugin-compression dependency and related transitive dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "semantic-release": "^24.1.2", | ||
| "typescript": "^5.8.3", | ||
| "vite": "^6.0.0", | ||
| "vite-plugin-compression": "^0.5.1", |
There was a problem hiding this comment.
The vite-plugin-compression version 0.5.1 is from 2022, which is significantly older than other dependencies in the project (e.g., vite is at ^6.0.0). Consider checking if there's a more recent version of vite-plugin-compression available that's compatible with Vite 6. Older plugins may not be fully compatible with newer Vite versions and could have unpatched security vulnerabilities or missing features.
Additionally, verify that this older plugin version is compatible with the Vite 6.0.0 being used in this project.
| "vite-plugin-compression": "^0.5.1", | |
| "vite-plugin-compression": "^0.6.0", |
| viteCompression({ | ||
| algorithm: 'brotliCompress', | ||
| ext: '.br', | ||
| filter: /\.(wasm|js)$/i, | ||
| threshold: 1024, // Only compress files > 1KB | ||
| deleteOriginFile: false, // Keep original files for compatibility | ||
| }), |
There was a problem hiding this comment.
The threshold value of 1024 bytes (1KB) for compression is quite aggressive. Brotli compression has overhead, and compressing very small files (1-2KB) may actually result in larger file sizes or negligible benefits.
Consider increasing the threshold to at least 10KB (10240 bytes) for WASM files and 5KB (5120 bytes) for JS files. This is a more common practice that ensures compression overhead doesn't outweigh the benefits for smaller files.
| viteCompression({ | |
| algorithm: 'brotliCompress', | |
| ext: '.br', | |
| filter: /\.(wasm|js)$/i, | |
| threshold: 1024, // Only compress files > 1KB | |
| deleteOriginFile: false, // Keep original files for compatibility | |
| }), | |
| // Compress WASM files > 10KB | |
| viteCompression({ | |
| algorithm: 'brotliCompress', | |
| ext: '.br', | |
| filter: /\.(wasm)$/i, | |
| threshold: 10240, // Only compress WASM files > 10KB | |
| deleteOriginFile: false, // Keep original files for compatibility | |
| }), | |
| // Compress JS files > 5KB | |
| viteCompression({ | |
| algorithm: 'brotliCompress', | |
| ext: '.br', | |
| filter: /\.(js)$/i, | |
| threshold: 5120, // Only compress JS files > 5KB | |
| deleteOriginFile: false, // Keep original files for compatibility | |
| }), |
| 'ducjs', // Workspace dependency | ||
| 'ducpdf', // Uses Rust WASM - let consuming app handle | ||
| 'ducjs', // Workspace dependency | ||
| /pdf2svg/, // Externalize pdf2svg wasm bindings to prevent inline base64 bundling |
There was a problem hiding this comment.
The regex pattern /pdf2svg/ will match any module path containing "pdf2svg" anywhere in the string. This might be overly broad and could unintentionally externalize modules that happen to have "pdf2svg" in their path but aren't the intended pdf2svg package.
Consider making the pattern more specific, such as /^pdf2svg/ to only match modules that start with "pdf2svg", or use a string 'pdf2svg' if you want to match the exact package name. The current pattern could match paths like 'node_modules/some-package/pdf2svg-utils/index.js' which may not be intended.
| /pdf2svg/, // Externalize pdf2svg wasm bindings to prevent inline base64 bundling | |
| 'pdf2svg', // Externalize pdf2svg wasm bindings to prevent inline base64 bundling |
| const ducDepRegex = /^(#[^\n]*\n)*duc\s*=\s*\{[^}]+\}/m; | ||
|
|
||
| if (!ducDepRegex.test(content)) { | ||
| console.error("Could not find duc dependency with version/path in Cargo.toml"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Replace the entire duc dependency (including any comment above it) with simple version | ||
| content = content.replace(ducDepRegex, `duc = "${version}"`); |
There was a problem hiding this comment.
The regex pattern /^(#[^\n]*\n)*duc\s*=\s*\{[^}]+\}/m may not handle all edge cases correctly:
- Multi-line duc dependency declarations with nested braces would fail (e.g., if there are features with nested config)
- The pattern
[^}]+is greedy and will match up to the first}, which might not be the correct closing brace if the dependency has nested structures - Comments with special characters or unicode might cause issues with
[^\n]*
While these edge cases may not occur in the current codebase, consider using a more robust TOML parser library (like @iarna/toml or toml) to parse, modify, and serialize the Cargo.toml file. This would be more maintainable and handle edge cases correctly.
| } | ||
|
|
||
| // Replace the entire duc dependency (including any comment above it) with simple version | ||
| content = content.replace(ducDepRegex, `duc = "${version}"`); |
There was a problem hiding this comment.
The script directly uses user input (process.argv[3]) in a string replacement without validation or sanitization. While the version parameter is expected to be a simple version string, a malicious or malformed input could potentially inject unexpected content into the Cargo.toml file.
Consider adding validation for the version parameter to ensure it matches expected version patterns (e.g., semver format like 1.2.3, 2, 1.0.0-beta, etc.) before using it in the replacement. For example:
if (!/^[\d.]+(-[a-zA-Z0-9.-]+)?$/.test(version)) {
console.error(`Invalid version format: ${version}`);
process.exit(1);
}This would prevent injection of malicious or malformed content into the Cargo.toml file.
| // 3. Build the project | ||
| prepareCmd: | ||
| "cargo set-version ${nextRelease.version} && cargo build --release", | ||
| "node ../../../../scripts/cargo-set-duc-dep-version.js . 2 && cargo set-version ${nextRelease.version} && cargo build --release", |
There was a problem hiding this comment.
The version "2" is hardcoded in the release script. This creates a tight coupling between the release configuration and the actual version of the 'duc' dependency available on crates.io. If the duc package version changes, this hardcoded value will need to be manually updated in the release config.
Consider making this configurable, either through an environment variable or by determining the version dynamically (e.g., reading from a configuration file or using a semantic version range like "^2.0.0" if appropriate for crates.io compatibility).
| "node ../../../../scripts/cargo-set-duc-dep-version.js . 2 && cargo set-version ${nextRelease.version} && cargo build --release", | |
| "node ../../../../scripts/cargo-set-duc-dep-version.js . ${DUC_VERSION:-2} && cargo set-version ${nextRelease.version} && cargo build --release", |
|
🎉 This PR is included in version 2.3.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.3.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.2.3 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 2.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
No description provided.