Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify proto JS API for tracker and griffin UI #1253

Merged
merged 19 commits into from
Nov 22, 2024

Conversation

atuchin-m
Copy link
Collaborator

@atuchin-m atuchin-m commented Nov 6, 2024

The PR reimplements finch_tracker and griffin.brave.com using protobuf-ts instead protobufjs
Also it reuses the serializer from seed_tools that will result in changing the format in finch-data-private

@atuchin-m atuchin-m force-pushed the another-proto-js-api-for-tracker branch from 3760af1 to 37cd0d1 Compare November 20, 2024 07:13
@atuchin-m atuchin-m self-assigned this Nov 20, 2024
@atuchin-m atuchin-m force-pushed the another-proto-js-api-for-tracker branch from 4624ac2 to 6236883 Compare November 20, 2024 11:07
@atuchin-m atuchin-m marked this pull request as ready for review November 20, 2024 11:08
@atuchin-m atuchin-m requested a review from a team as a code owner November 20, 2024 11:08
@atuchin-m atuchin-m requested a review from goodov November 20, 2024 11:08
src/finch_tracker/tracker_lib.ts Outdated Show resolved Hide resolved
src/seed_tools/utils/study_json_utils.ts Outdated Show resolved Hide resolved
);
}
case 'platform': {
return value.map((p: string) => platformToString(p));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before this change the logic for revive and replace was in the same file close to each other. Now it's spread a bit. Can we do something here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/seed_tools/utils/study_json_utils.test.ts Show resolved Hide resolved
src/core/url_utils.ts Show resolved Hide resolved
Copy link
Contributor

[puLL-Merge] - brave/brave-variations@1253

Description

This PR makes significant changes to the brave-variations project, primarily focusing on updating the protobuf implementation and refactoring the codebase to use a new protobuf-ts library instead of the previously used protobufjs. The changes also include updates to file processing, serialization, and various utility functions.

Possible Issues

  1. The removal of protobufjs and protobufjs-cli dependencies might break existing functionality if they were used elsewhere in the project.
  2. The changes in serialization and processing logic might lead to unexpected behavior in downstream components that rely on the previous implementation.

Security Hotspots

No significant security hotspots were identified in this PR.

Changes

Changes

  1. package.json:

    • Removed dependencies: protobufjs and protobufjs-cli
  2. src/core/serializers.ts:

    • Removed entire file
  3. src/core/study_processor.ts:

    • Updated imports to use new proto generated files
    • Refactored code to use new protobuf-ts types and methods
    • Updated platform and channel constants
  4. src/core/summary.ts:

    • Updated imports to use new proto generated files
  5. src/core/url_utils.ts:

    • Updated URL construction for study raw config
  6. src/finch_tracker/main.ts:

    • Updated protobuf decoding to use new fromBinary method
  7. src/finch_tracker/tracker_lib.ts:

    • Refactored storeDataToDirectory function to use new protobuf-ts methods
    • Updated groupStudies function to work with new Study type
  8. src/scripts/generate_proto_ts.ts:

    • Removed generateProtobufJsWithTypeInfo function
    • Updated generateProtobufTs function
  9. src/seed_tools/utils/serializers.ts:

    • Added new file with channelToString and platformToString functions
  10. src/seed_tools/utils/study_json_utils.ts:

    • Updated JSON parsing and stringification to work with new protobuf-ts types
  11. src/web/app/*.tsx and *.ts files:

    • Updated imports to use new proto generated files
    • Refactored code to work with new protobuf-ts types and methods
  12. src/test/data/seed1.bin.processing_expectations:

    • Updated expected output format
sequenceDiagram
    participant Web as Web App
    participant Core as Core
    participant Proto as Proto Generated
    participant Seed as Seed Tools
    participant Finch as Finch Tracker

    Web->>Core: Use study processor
    Core->>Proto: Import new types
    Web->>Seed: Use serializers
    Seed->>Proto: Import new types
    Finch->>Core: Use summary
    Finch->>Proto: Use new fromBinary method
    Core->>Seed: Use new serializer functions
Loading
graph TD
    A[Web App] --> B[Core]
    A --> C[Seed Tools]
    B --> D[Proto Generated]
    C --> D
    B --> E[Finch Tracker]
    E --> D
    F[Scripts] --> D
Loading

C4 Diagram

Sequence Diagram

@atuchin-m atuchin-m added this pull request to the merge queue Nov 22, 2024
Merged via the queue into main with commit 5c7eb50 Nov 22, 2024
5 checks passed
@atuchin-m atuchin-m deleted the another-proto-js-api-for-tracker branch November 22, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants