Skip to content

chore: remove udb as a dependency of envoy-client#4587

Open
NathanFlurry wants to merge 1 commit into04-07-chore_revert_actor_v2_hackfrom
04-07-chore_remove_udb_as_a_dependency_of_envoy-client
Open

chore: remove udb as a dependency of envoy-client#4587
NathanFlurry wants to merge 1 commit into04-07-chore_revert_actor_v2_hackfrom
04-07-chore_remove_udb_as_a_dependency_of_envoy-client

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@claude
Copy link
Copy Markdown

claude bot commented Apr 8, 2026

Code Review

This PR extracts serde utilities from rivet-util into a new dedicated rivet-util-serde crate so envoy-client no longer drags in the heavy rivet-util dependency tree. The overall approach is sound.

Issues

Missing workspace registration for the new crate

engine/packages/util-serde is added as a new package, but the root Cargo.toml is not updated to include it in either the members list or [workspace.dependencies]. Without these two additions, the package will not build as part of the workspace, and downstream crates using rivet-util-serde.workspace = true will get a "package not found" error.

The diff needs:

# members list (around line 49)
"engine/packages/util-serde",

# workspace.dependencies section
[workspace.dependencies.rivet-util-serde]
path = "engine/packages/util-serde"

build.rs post-processing is fragile

post_process_generated_rust does a raw string replacement of rivet_util::serde::HashableMap in generated files. This approach has a few risks:

  • It mutates OUT_DIR files after the code generator runs. If vbare_compiler is ever updated to emit the new path directly this becomes a silent no-op, but if it emits the old path in a context other than a bare identifier (e.g. inside a use statement, a doc comment, or a qualified path like rivet_util::serde::HashableMap::new) the replacement could produce malformed code.
  • The better long-term fix is to configure vbare_compiler to emit rivet_util_serde::HashableMap instead, or alias the type in the generated files via a use shim. The build-time post-processing should carry a comment explaining why it exists and when it can be removed.

thiserror is still in util/Cargo.toml removal scope

The PR removes thiserror from rivet-util's Cargo.toml, but thiserror may still be used elsewhere in rivet-util (e.g. other error types in engine/packages/util/src/). It is worth confirming that no remaining code in that crate relies on thiserror before removing it.

Minor Notes

  • The commented-out ToSchema/PartialSchema impl block is preserved in the new crate. It has a // TODO: This doesn't work comment. If this is tracked elsewhere it is fine to leave as-is, but it would be cleaner to either delete it or convert it to a tracked issue.
  • FakeMap::sort has a redundant where K: Ord bound that duplicates the impl-level bound. This was pre-existing in rivet-util, so not introduced here, but worth noting.
  • The new util-serde/src/lib.rs uses ::serde:: (with leading ::) as a qualifier in a few places but plain serde:: in others. Minor style inconsistency, also pre-existing.

Summary

The extraction itself is clean and the import/re-export pattern (pub use rivet_util_serde::* in the old module) maintains backward compatibility for existing callers via rivet-util. The critical blocker is the missing workspace registration; the build will fail without it.

@NathanFlurry NathanFlurry mentioned this pull request Apr 8, 2026
11 tasks
@NathanFlurry NathanFlurry marked this pull request as ready for review April 8, 2026 10:37
@NathanFlurry NathanFlurry mentioned this pull request Apr 8, 2026
11 tasks
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.

1 participant