Skip to content

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Nov 4, 2025

Description:

Bump command should not bump the versions of binding crates, because those are managed by https://github.com/swc-project/swc/blob/2edbd40241b3402c8542241b1f8d82a4ebadd801/scripts/publish.sh .

But before this PR, there was a bug related to packages that are not published to crates.io, so there was an issue in a commit like 5e6af60

@kdy1 kdy1 added this to the Planned milestone Nov 4, 2025
@kdy1 kdy1 requested a review from a team as a code owner November 4, 2025 03:27
Copilot AI review requested due to automatic review settings November 4, 2025 03:27
@changeset-bot
Copy link

changeset-bot bot commented Nov 4, 2025

⚠️ No Changeset found

Latest commit: b806cf2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@claude
Copy link
Contributor

claude bot commented Nov 4, 2025

Claude finished @kdy1's task —— View job


Code Review for PR #11226

Reviewing the changes to improve the bump command to exclude binding crates.

Todo List:

  • Read and analyze the modified file
  • Review code quality and implementation
  • Check for potential bugs or issues
  • Assess performance and security considerations
  • Provide comprehensive feedback

Review Summary

This PR makes two key improvements to the bump command in tools/swc-releaser/src/main.rs:

Positive Changes

  1. Fixed Non-Published Package Handling (tools/swc-releaser/src/main.rs:337-340):

    for pkg in md.workspace_packages() {
        if pkg.publish == Some(vec![]) {
            continue;
        }
        versions.insert(pkg.name.clone(), pkg.version.clone());
    }
    • Good: Correctly excludes packages with publish = false from version tracking
    • Addresses the Issue: This fixes the bug mentioned in the PR description where non-published packages were causing issues
  2. Graceful Missing Version Handling (tools/swc-releaser/src/main.rs:213-216):

    let Some(original_version) = self.versions.get(pkg_name) else {
        eprintln!("No original version found for {pkg_name}, skipping bump");
        return Ok(());
    };
    • Good: Replaces a potential panic with graceful handling
    • Better UX: Provides clear logging when a package is skipped
    • Defensive Programming: Uses modern Rust let-else syntax for cleaner error handling

🔍 Code Quality Assessment

Strengths:

  • Clean, idiomatic Rust code using modern patterns (let-else)
  • Consistent error handling approach
  • Clear, informative logging messages
  • Maintains existing function signatures and behavior for published packages

Minor Considerations:

  • The logic is sound and follows the existing codebase patterns
  • No performance implications from these changes
  • Memory usage remains the same (filtering happens early)

🛡️ Security & Safety

  • No security concerns: Changes are purely logical improvements
  • Error handling: Improves robustness by handling edge cases gracefully
  • No unsafe code: All changes use safe Rust patterns

🧪 Test Coverage

The changes are defensive improvements that should be backwards compatible. However, consider adding tests for:

  • Packages with publish = false are properly excluded
  • Missing version handling works correctly
  • Ensure binding crates are not bumped in integration scenarios

📈 Performance Impact

  • Minimal positive impact: Early filtering of non-published packages reduces unnecessary processing
  • No performance degradation: The changes add minimal overhead

Overall Assessment

This is a well-implemented fix that addresses the specific issue described in the PR. The changes are:

  • ✅ Targeted and focused
  • ✅ Use modern Rust idioms
  • ✅ Maintain backwards compatibility
  • ✅ Include proper error handling and logging
  • ✅ Address the root cause of the binding crates issue

Recommendation: Approve - These changes correctly implement the intended functionality to exclude binding crates from version bumping while improving error handling.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves handling of unpublishable packages in the SWC releaser tool. The changes ensure that packages marked with publish = false (represented as an empty vector in cargo_metadata) are properly excluded from version bumping operations.

  • Filters out unpublishable packages when building the version map
  • Changes error handling to gracefully skip packages without versions instead of failing
  • Prevents version bumping attempts on packages that should not be published
Comments suppressed due to low confidence (1)

tools/swc-releaser/src/main.rs:188

  • The is_breaking method still uses the old error handling pattern and will fail if a package is not in the versions map. This is inconsistent with the graceful handling added to bump_crate. If a changeset references an unpublishable package, is_breaking will error before bump_crate gets a chance to skip it gracefully. Consider applying the same pattern here: return early with a default value (e.g., false) when the package is not found in the versions map.
    fn is_breaking(&self, pkg_name: &str, change_type: Option<&ChangeType>) -> Result<bool> {
        let original_version = self
            .versions
            .get(pkg_name)
            .context(format!("failed to find original version for {pkg_name}"))?;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2025

Binary Sizes

File Size
swc.linux-x64-gnu.node 31M (31905672 bytes)

Commit: 0c4e28e

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 4, 2025

CodSpeed Performance Report

Merging #11226 will not alter performance

Comparing kdy1/bump (b806cf2) with main (af134fa)1

Summary

✅ 138 untouched

Footnotes

  1. No successful run was found on main (2edbd40) during the generation of this report, so af134fa was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@kdy1 kdy1 merged commit 2d33124 into main Nov 4, 2025
189 checks passed
@kdy1 kdy1 deleted the kdy1/bump branch November 4, 2025 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants