Connect: Generate json schema for app config

[gzdunek]

Part of #21914

This PR adds JSON schema generation for our config schema (and slightly extends the FileStorage API).
I was considering using fs.write* API in the updateJsonSchema to write the file, but I think using a FileStorage object has more advantages:

• In updateJsonSchema we would need a path to the user data dir, probably the easiest way would to be pass the entire RuntimeSettings there; with FileStorage we can avoid it.
• The json schema is a JS object that has to be stringified to JSON (in a pretty way, if possible) - FileStorage does both.
• We keep files we write to in one place.
• Testing is easier IMO, we don't have to mock fs functions.

[gzdunek]

If the config file has a syntax error, this will cause overriding it with an empty object containing only $schema field.
I will handle this case in the next PR.

[ravicious]

Wow, editing the config with this feels like magic. My VSCode schema detection is broken and it always puts the same default everywhere but my vim plugin works as expected.

I left some comments regarding the structure of the code because my mental fortitude this week is super weak and realizing why you need to have a getter for the file path on the file storage took me way to long to realize and I had to jump between too many files! I agree with the decision regarding getFilePath described in the PR description though.

I might have some more comments next week.

[gzdunek]

I noticed that subsequent keyboard shortcuts refer to the first one. That's because we are reusing the same schema object for them.

 "keymap.tab1": {
      "type": "string",
      "default": "Command+1",
      "description": "Shortcut to open tab 1. A valid shortcut contains at least one modifier and a single key code, for example \"Shift+Tab\". Function keys do not require a modifier."
    },
    "keymap.tab2": {
      "$ref": "#/properties/keymap.tab1",
      "default": "Command+2",
      "description": "Shortcut to open tab 2. A valid shortcut contains at least one modifier and a single key code, for example \"Shift+Tab\". Function keys do not require a modifier."
    },

I think it looks quite weird, so we should either:

• go back to calling getKeyboardShortcut() for each config key, or
• set { $refStrategy: 'none' } for zodToJsonSchema

IMO the second option is fine.

[ravicious]

I'm also partial to the second option.

Correct me if I'm wrong, but I've assumed that you can compose zod schemas however you want into new schemas – I mean that's how the API of zod is structured, you have things like z.string().or(z.number()). So IMHO it only follows that calling .default on an existing schema creates a new one. Perhaps zod-to-json-schema tries to be a little too smart here, possibly because other than the default value the schemas are structurally the same.

[gzdunek]

Yeah, I think it is a smart mechanism that looks for the same references (?) in the schema:

Resolves recursive and recurring schemas with internal $refs.

(and AFAIK we can compose schemas as we want)

[ravicious]

Off topic nit but lately I've been reconsidering if putting the verbs in the names of the files themselves is a good idea. I feel like inside this dir we could have appConfigSchema.ts and keyboardShortcutSchema.ts and it'd be totally fine. We also don't have that many files starting with get/create in the project.

It'd also help clean up the naming between createAppConfigSchema and getKeyboardShortcutSchema, at least on the file name level. 😏 I think I prefer the verb "create" or "make" for this scenario over "get" as what we're really doing here is instantiating something new.

[gzdunek]

I renamed getKeyboardShortcutSchema too :)

I don't have a strong opinion on the file names, and even in a services/config/ module we were inconsistent, for example a function createConfigService was exported from configService.ts file.

Sometimes naming the file the same as a main/only export makes sense for me, for example in tshd/createClient.ts (maybe I'm just used to it). Would you see it as tshd/client.ts?

[ravicious]

Would you see it as tshd/client.ts?

I suppose so? I don't have a hard opinion on this atm, but overall I'm thinking about shifting focus when naming from particular functions to overall concepts.

If JS was more opinionated about imports and files then it'd all be easier. For example each file in Elm is a separate module with a specific namespace, so you could have ConfigService.elm and you'd expect to find a function called ConfigService.init. Inside ConfigService.elm, you can use init without the namespace qualifier. Outside of that module, you typically import the whole module and call ConfigService.init but you can also import a specific function without the namespace qualifier or use an alias for the namespace.

In JS there's no notion of this namespace which makes things harder.

---

In general I think I'd aim at reducing the number of files we create, each function doesn't have to live in a separate file. Take a look at e/web/teleport/src/services/recovery/makeRecoveryToken.ts, this function could as well live in e/web/teleport/src/services/recovery/recovery.ts.

Bigger functions such as web/packages/teleport/src/services/recordings/makeRecording.ts could live in recording.ts as well, especially since they define functions that are related to a recording but might be useful outside of the context of makeRecording, for example formatDuration.

This all seems easy and clear as long as we we're talking about non-React code. As soon as we enter React then we potentially have a conflict between recovery.ts and a file for a component called Recovery.

[public-teleport-github-review-bot]

@gzdunek See the table below for backport results.

Branch	Result
branch/v12	Failed