Skip to content

Fix Deserialize derive for BrpResponse#24305

Merged
alice-i-cecile merged 5 commits into
bevyengine:mainfrom
MarcGuiselin:deserializable-brp-response
Jun 17, 2026
Merged

Fix Deserialize derive for BrpResponse#24305
alice-i-cecile merged 5 commits into
bevyengine:mainfrom
MarcGuiselin:deserializable-brp-response

Conversation

@MarcGuiselin

@MarcGuiselin MarcGuiselin commented May 15, 2026

Copy link
Copy Markdown
Contributor

Objective

Bevy remote's BrpResponse derives serde::Deserialize, but the static lifetime prevents it from actually being so:

let response: BrpResponse = serde_json::from_str(&response)?;
                            ---------------------^^^^^^^^^-
                            |                    |
                            |                    borrowed value does not live long enough
                            argument requires that `response` is borrowed for `'static`

No migration guide needed, since the interface for BrpResponse::new used by downstream implementers remains unchanged.

Solution

Remove the jsonrpc field by writing a custom impl for Serialize & Deserialize, which is the same solution as done previously in #23175.

Testing

This change has a very low impact. Tests pass.

Alternatives

  1. Replace jsonrpc with String
  2. Replace jsonrpc with Cow

@MarcGuiselin MarcGuiselin changed the title Fix Deserializable BrpResponse Fix Deserialize derive for BrpResponse May 15, 2026
@kfc35 kfc35 added S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Dev-Tools Tools used to debug Bevy applications. D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 16, 2026
@SpecificProtagonist

Copy link
Copy Markdown
Contributor

This works, but I think the better solution is to do the same as #23175 and remove the jsonrpc field by writing a custom Serialize&Deserialize implementation.

@SpecificProtagonist SpecificProtagonist added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 26, 2026
@MarcGuiselin

Copy link
Copy Markdown
Contributor Author

@SpecificProtagonist Done. Sorry for the hiatus, but now this PR should be unblocked from me.

@MarcGuiselin

Copy link
Copy Markdown
Contributor Author

@alice-i-cecile I'd be nice to land this in 0.19, since this pr compliments #23175 and corrects some outdated docs leftover from that one

Comment thread crates/bevy_remote/src/lib.rs Outdated
Co-authored-by: Mira <specificprotagonist@posteo.org>
@SpecificProtagonist SpecificProtagonist added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 13, 2026
@alice-i-cecile alice-i-cecile added this to the 0.19 milestone Jun 15, 2026
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 17, 2026
Merged via the queue into bevyengine:main with commit 96e198e Jun 17, 2026
40 checks passed
alice-i-cecile pushed a commit that referenced this pull request Jun 18, 2026
# Objective

Bevy remote's `BrpResponse` derives `serde::Deserialize`, but the static
lifetime prevents it from actually being so:

```
let response: BrpResponse = serde_json::from_str(&response)?;
                            ---------------------^^^^^^^^^-
                            |                    |
                            |                    borrowed value does not live long enough
                            argument requires that `response` is borrowed for `'static`
```

No migration guide needed, since the interface for `BrpResponse::new`
used by downstream implementers remains unchanged.

## Solution

Remove the `jsonrpc` field by writing a custom `impl` for `Serialize` &
`Deserialize`, which is the same solution as done previously in #23175.

## Testing

This change has a very low impact. Tests pass.

## Alternatives

1. Replace jsonrpc with `String`
2. Replace jsonrpc with `Cow`

---------

Co-authored-by: Mira <specificprotagonist@posteo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Dev-Tools Tools used to debug Bevy applications. D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants