This repository has been archived by the owner on Feb 3, 2023. It is now read-only.
Serialize ConsistencySignal before transmitting across signal channel
#1691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maackle
force-pushed
the
consistency-serialization
branch
from
c865767
to
6a99fed
Compare
maackle
commented
Sep 3, 2019
| @@ -10,7 +10,7 @@ use std::thread; | |||
| #[serde(tag = "signal_type")] | |||
| pub enum Signal { | |||
| Trace(ActionWrapper), | |||
| Consistency(ConsistencySignal), | |||
| Consistency(ConsistencySignal<String>), | |||
There was a problem hiding this comment.
This, and the new From impl which goes from enum to String, is the main crux of the PR.
maackle
commented
Sep 3, 2019
| .collect(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the main new part, most of the rest of the PR is just moving stuff around.
StaticallyTypedAnxiety
approved these changes
Sep 3, 2019
zippy
approved these changes
Sep 3, 2019
maackle
force-pushed
the
consistency-serialization
branch
from
22d9740
to
bb8fc58
Compare
|
NB: I had to rerun tests 3 times to get this to pass |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR summary
Hachiko expects observable "events" as strings which match up exactly, coming from different sources at different times. Currently the serialization is happening on the JS side via
JSON.stringify, but this is not great sinceJSON.stringifyis not deterministic. This PR uses serde to do the serialization, under the assumption that serde_json is deterministic.testing/benchmarking notes
This will probably break current app spec tests since try-o-rama currently does the serialization itself. I need to update the npm package for try-o-rama to not do this for this PR.
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)