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

Use Typescript for config schemas #2439

Merged
merged 20 commits into from
Nov 11, 2024
Merged

Use Typescript for config schemas #2439

merged 20 commits into from
Nov 11, 2024

Conversation

sammacbeth
Copy link
Collaborator

Asana Task/Github Issue: https://app.asana.com/0/481882893211075/1208516641945153/f

Description

  • Defines the full config schema in TS.
  • Updates tests to validate v4 and v3 generated configs against this schema.
  • Replaces all existing JSONSchema definitions with TS equivalents, and fixes missing values where validation found them. Updates those tests to use the TS validator.

Reference

Copy link

github-actions bot commented Nov 8, 2024

Generated file outputs:

Time updated: Fri, 08 Nov 2024 16:55:38 GMT

legacy
trackers-unprotected-temporary.txt (14 more)
  • trackers-unprotected-temporary.txt
  • v3/android-config.json
  • v3/extension-brave-config.json
  • v3/extension-bravemv3-config.json
  • v3/extension-chrome-config.json
  • v3/extension-chromemv3-config.json
  • v3/extension-config.json
  • v3/extension-edg-config.json
  • v3/extension-edge-config.json
  • v3/extension-edgmv3-config.json
  • v3/extension-firefox-config.json
  • v3/extension-safarimv3-config.json
  • v3/ios-config.json
  • v3/macos-config.json
  • v3/windows-config.json

⚠️ File is identical

latest
v4/android-config.json (13 more)
  • v4/android-config.json
  • v4/extension-brave-config.json
  • v4/extension-bravemv3-config.json
  • v4/extension-chrome-config.json
  • v4/extension-chromemv3-config.json
  • v4/extension-config.json
  • v4/extension-edg-config.json
  • v4/extension-edge-config.json
  • v4/extension-edgmv3-config.json
  • v4/extension-firefox-config.json
  • v4/extension-safarimv3-config.json
  • v4/ios-config.json
  • v4/macos-config.json
  • v4/windows-config.json

⚠️ File is identical

@sammacbeth sammacbeth changed the title Sam/ts schema Use Typescript for config schemas Nov 8, 2024
@jonathanKingston
Copy link
Collaborator

Before we merge this I want to make sure C-S-S doesn't explode. We had some auto routing of the types so the getSettings code just magically worked with these settings types.

No real issues otherwise from a quick glance.

@muodov muodov self-requested a review November 8, 2024 12:22
Copy link
Collaborator

@jonathanKingston jonathanKingston left a comment

Choose a reason for hiding this comment

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

LGTM, I played around with it a bit and also linked it into C-S-S fairly easily (no other consumers exist to these types that I'm aware of).

There's a bunch of fast follows like:

  1. Typing out some of the C-S-S stuff to be shared a bit better to help feature authors with the domain patching stuff but that's certainly not a blocker as we didn't have anything here.
  2. I think we can remove both duckplayer and webcompat test now this change links the testing in automatically for those settings. (Happy to remove these as a follow up if that's the case).

On risk to merge:

  1. It looks like you've copied over what typing we already had (and added more).
    Given that until recently we weren't even blocking merges on tests passing; we can take the risk on some edge cases of these being a little looser than they might have been (I couldn't spot any other than the feature name that you fixed :) ).
  2. Additionally if the typing is slightly too restrictive, I think it's not a huge concern either.

Feel free to merge when ready 👍🏻

@sammacbeth sammacbeth enabled auto-merge November 11, 2024 08:51
@sammacbeth sammacbeth added this pull request to the merge queue Nov 11, 2024
Merged via the queue into main with commit c319b30 Nov 11, 2024
11 checks passed
@sammacbeth sammacbeth deleted the sam/ts-schema branch November 11, 2024 09:49
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.

2 participants