Skip to content

Conversation

@kixelated
Copy link
Owner

@kixelated kixelated commented Oct 17, 2025

Nobody is (currently) using the trait, and supporting it breaks rust-analyzer and release-plz.

Summary by CodeRabbit

  • Refactor

    • Streamlined WASM transport module by simplifying internal architecture while maintaining all existing functionality.
  • Chores

    • Updated build verification to consistently include all packages in standard checks.
    • Cleaned up unused internal workspace dependencies.

@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025

Walkthrough

The changes remove the web_transport_trait dependency and all corresponding trait implementations from the web-transport-wasm package. Specifically, trait implementations are removed from the Error, RecvStream, SendStream, and Session types in separate source files. The web-transport-trait dependency is removed from Cargo.toml. The compile-time WASM platform guard is removed from session.rs. Build configuration updates remove the explicit exclusion of web-transport-wasm from cargo check, test, and clippy operations, allowing these tools to operate across all packages by default.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Remove the trait support for WASM so it compiles without WASM" is directly related to the main changes in the pull request. The changeset removes trait implementations (web_transport_trait::Error, RecvStream, SendStream, and Session) from the web-transport-wasm package, removes the web-transport-trait dependency, and removes the WASM-only compile guard, which together enable the module to compile on non-WASM targets. The title accurately captures the primary action (removing trait support) and its consequence (enabling compilation without WASM), though it doesn't explicitly mention the underlying motivation stated in the PR description (fixing rust-analyzer and release-plz compatibility).
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch no-wasm-trait

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6b00d29 and 4d9c542.

📒 Files selected for processing (6)
  • justfile (3 hunks)
  • web-transport-wasm/Cargo.toml (0 hunks)
  • web-transport-wasm/src/error.rs (0 hunks)
  • web-transport-wasm/src/recv.rs (0 hunks)
  • web-transport-wasm/src/send.rs (0 hunks)
  • web-transport-wasm/src/session.rs (0 hunks)
💤 Files with no reviewable changes (5)
  • web-transport-wasm/src/error.rs
  • web-transport-wasm/src/recv.rs
  • web-transport-wasm/Cargo.toml
  • web-transport-wasm/src/send.rs
  • web-transport-wasm/src/session.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (4)
justfile (4)

37-37: LGTM - Feature-powerset check now includes web-transport-wasm.

Consistent with the other workspace-wide changes. The feature-powerset check will help ensure the package compiles correctly with all feature combinations on non-WASM targets.


49-49: LGTM - Test suite now includes web-transport-wasm.

The workspace-wide test command now includes web-transport-wasm, with WASM-specific tests retained on lines 50-51. This ensures the package is tested on both WASM and non-WASM targets.


55-56: LGTM - Fix commands now include web-transport-wasm.

Consistent with the other changes. The fix commands will now operate across all workspace packages, with WASM-specific fixes retained on lines 59-62 for explicit wasm32-unknown-unknown target checks.


24-25: LGTM - Workspace now includes web-transport-wasm in standard checks.

The removal of --exclude web-transport-wasm from workspace-wide commands (lines 24-25, 37, 49, 55-56) is syntactically correct and consistent across all affected cargo operations. The retained WASM-specific checks with explicit --target wasm32-unknown-unknown (lines 28-31, 38-39, 50-51, 59-62) ensure cross-platform compatibility.

Verification in the sandbox environment failed due to environment limitations. Please manually verify that web-transport-wasm compiles successfully on the default (non-WASM) target:

cargo check -p web-transport-wasm --all-targets --all-features
cargo clippy -p web-transport-wasm --all-targets --all-features -- -D warnings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated kixelated enabled auto-merge (squash) October 17, 2025 22:38
@kixelated kixelated merged commit c47a91e into main Oct 17, 2025
1 check passed
@kixelated kixelated deleted the no-wasm-trait branch October 17, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants