mozilla
Move the bso_record and key_bundle into the sync15_traits crate.
(Which further reinforces that the name of that crate should be something like
sync15-types)
This is preparatory work for a much larger patch that will get us further down the path to #2841 and also fix #2712. It should not change any behaviour.
As part of this, the sync15_traits errors needs to be addressed. It's not reasonable to just create another enum that we expect clients to add to their error types because many of the errors overlap which makes handling them very difficult (eg, if we want to catch a serde error, you don't want to know which crate it originated in) - so sync15 duplicates the sync15_traits errors and can convert between them - so consumers can continue to catch sync15::Error and still get errors which originated in sync15-traits.
sync15-traits now has a crypto feature - sync15 enables that feature, but
crates which just use a bridged_engine do not. This means that when
vendoring into Desktop Firefox, this feature isn't enabled, so we don't
pull in any of the crypto libs there.
@tarikeshaq and @bendk, I'd welcome all feedback here, particularly around the error changes. They make sense to me, but I might be missing something! Note that I'll probably hold off merging this until that larger patch is up for initial review.
Pull Request checklist
- [ ] Quality: This PR builds and tests run cleanly
- Note:
- For changes that need extra cross-platform testing, consider adding
[ci full]to the PR title. - If this pull request includes a breaking change, consider cutting a new release after merging.
- For changes that need extra cross-platform testing, consider adding
- Note:
- [ ] Tests: This PR includes thorough tests or an explanation of why it does not
- [ ] Changelog: This PR includes a changelog entry in CHANGES_UNRELEASED.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
- [ ] Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due dilligence applied in selecting them.
Branch builds: add [ac: android-components-branch-name] and/or [fenix: fenix-branch-name] to the PR title.
Codecov Report
Merging #5083 (2edfc4d) into main (ce31bcd) will decrease coverage by
0.12%. The diff coverage is0.00%.
@@ Coverage Diff @@
## main #5083 +/- ##
==========================================
- Coverage 38.53% 38.41% -0.13%
==========================================
Files 168 169 +1
Lines 12364 12404 +40
==========================================
Hits 4765 4765
- Misses 7599 7639 +40
| Impacted Files | Coverage Δ | |
|---|---|---|
| components/fxa-client/src/internal/error.rs | 0.00% <0.00%> (ø) |
|
| components/support/sync15-traits/src/bso_record.rs | 0.00% <ø> (ø) |
|
| components/support/sync15-traits/src/error.rs | 0.00% <0.00%> (ø) |
|
| components/support/sync15-traits/src/key_bundle.rs | 0.00% <0.00%> (ø) |
|
| components/support/sync15-traits/src/lib.rs | 0.00% <ø> (ø) |
|
| components/sync15/src/changeset.rs | 0.00% <0.00%> (ø) |
|
| components/sync15/src/client.rs | 0.00% <ø> (ø) |
|
| components/sync15/src/clients/engine.rs | 0.00% <ø> (ø) |
|
| components/sync15/src/coll_state.rs | 0.00% <ø> (ø) |
|
| components/sync15/src/collection_keys.rs | 0.00% <0.00%> (ø) |
|
| ... and 7 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I need to understand the sync15 / sync15-traits split better to comment on this one. Couple questions here:
- What was the motivation behind the split? I thought there was a document about this, but I couldn't find anything. Also, is the split still needed nowaday? I see a note mentioning that maybe the sync15 functionality could be moved into either sync-manager or sync15-traits.
- Should a component depend on both sync15 and sync15-traits? It seems like it would be simpler if components only depended on one or the other.
I need to understand the sync15 / sync15-traits split better to comment on this one. Couple questions here:
* What was the motivation behind the split? I thought there was a document about this, but I couldn't find anything. Also, is the split still needed nowaday?
The split was made to avoid pulling in code where it wasn't needed and to avoid circular dependencies. eg:
- None of our components depend on sync-manager - instead, sync-manager depends on them (and also on sync15)
- All our components depend on sync15 (and indirectly sync15-traits) - but after we drop stand-alone syncing (possible once iOS uses the sync-manager), they can probably just depend on sync15-traits.
- Desktop syncing doesn't depend on either sync-manager or sync15.
So in today's world I don't think we can easily merge them.
- In a future world (no stand-alone syncing), it might be possible to merge sync-manager and sync15.
- It might also be possible to merge sync15 and sync15-traits with more aggressive features to avoid pulling unused code into desktop (currently webext-storage does not depend on sync15, so combining sync15 and sync15-traits would need to be done in a way that didn't introduce unused code to desktop)
But those 2 options are mutually-exclusive - we can't merge all 3 together or there will be circular dependencies between the sync engine implementations and the newly combined crate.
I see a note mentioning that maybe the sync15 functionality could be moved into either sync-manager or sync15-traits.
I can see a future where we can do some consolidation, but I'm not sure that now is the time. Hence, my patch doesn't really change the status-quo that much - that status-quo does need to change, but it's not 100% clear how and that's a bigger job than was trying to take on here :)
* Should a component depend on both sync15 and sync15-traits? It seems like it would be simpler if components only depended on one or the other.
Yeah, but that's already somewhat true today - eg, neither the tabs component nor the sync-manager components, both touched by this PR, have a direct dependency on sync15-traits. Only webext-storage needs to take an explicit dependency on sync15-traits because it doesn't depend on sync15. (autofill also takes an explicit dep on sync15-traits, but I bet it doesn't need to but was instead a copy-paste error)
It might also be possible to merge sync15 and sync15-traits with more aggressive features to avoid pulling unused code into desktop (currently webext-storage does not depend on sync15, so combining sync15 and sync15-traits would need to be done in a way that didn't introduce unused code to desktop)
This seems like it would simplify the error handling since you would only need 1 enum at that point. But I'm not sure how high of a priority this is.