feat: generic wrapper for JSON APIs#70
Draft
ExampleWasTaken wants to merge 37 commits intoflybywiresim:stagingfrom
Draft
feat: generic wrapper for JSON APIs#70ExampleWasTaken wants to merge 37 commits intoflybywiresim:stagingfrom
ExampleWasTaken wants to merge 37 commits intoflybywiresim:stagingfrom
Conversation
Collaborator
|
I like this in general, but needs a deeper review than I can do right now :) |
benw202
approved these changes
Jul 18, 2024
Member
benw202
left a comment
There was a problem hiding this comment.
LGTM, will wait on a second review from @pdellaert on this one
bca310f to
01e424d
Compare
pdellaert
reviewed
Jul 28, 2024
01e424d to
dec8382
Compare
# Conflicts: # .github/CHANGELOG.md
dec8382 to
aa84fa9
Compare
pdellaert
approved these changes
Nov 11, 2024
Collaborator
pdellaert
left a comment
There was a problem hiding this comment.
Only one tiny thing that is not a blocker to me. Otherwise looks really good
Sorry it took me so long to review
Comment on lines
+3
to
+24
| export const MetarTimeSchema = z.object({ dt: z.string().datetime() }); | ||
|
|
||
| export const MetarWindDirectionSchema = z.object({ repr: z.string() }); | ||
|
|
||
| export const MetarWindSpeedSchema = z.object({ repr: z.string() }); | ||
|
|
||
| export const MetarVisibilitySchema = z.object({ repr: z.string() }); | ||
|
|
||
| export const MetarTemperatureSchema = z.object({ repr: z.string() }); | ||
|
|
||
| export const MetarDewpointSchema = z.object({ repr: z.string() }); | ||
|
|
||
| export const MetarAltimeterSchema = z.object({ value: z.number() }); | ||
|
|
||
| export const MetarUnitsSchema = z.object({ | ||
| accumulation: z.string(), | ||
| altimeter: z.string(), | ||
| altitude: z.string(), | ||
| temperature: z.string(), | ||
| visibility: z.string(), | ||
| wind_speed: z.string(), | ||
| }); |
Collaborator
There was a problem hiding this comment.
Suggested change
| export const MetarTimeSchema = z.object({ dt: z.string().datetime() }); | |
| export const MetarWindDirectionSchema = z.object({ repr: z.string() }); | |
| export const MetarWindSpeedSchema = z.object({ repr: z.string() }); | |
| export const MetarVisibilitySchema = z.object({ repr: z.string() }); | |
| export const MetarTemperatureSchema = z.object({ repr: z.string() }); | |
| export const MetarDewpointSchema = z.object({ repr: z.string() }); | |
| export const MetarAltimeterSchema = z.object({ value: z.number() }); | |
| export const MetarUnitsSchema = z.object({ | |
| accumulation: z.string(), | |
| altimeter: z.string(), | |
| altitude: z.string(), | |
| temperature: z.string(), | |
| visibility: z.string(), | |
| wind_speed: z.string(), | |
| }); | |
| export const MetarTimeSchema = z.object({ dt: z.string().datetime() }); | |
| export const MetarWindDirectionSchema = z.object({ repr: z.string() }); | |
| export const MetarWindSpeedSchema = z.object({ repr: z.string() }); | |
| export const MetarVisibilitySchema = z.object({ repr: z.string() }); | |
| export const MetarTemperatureSchema = z.object({ repr: z.string() }); | |
| export const MetarDewpointSchema = z.object({ repr: z.string() }); | |
| export const MetarAltimeterSchema = z.object({ value: z.number() }); | |
| export const MetarUnitsSchema = z.object({ | |
| accumulation: z.string(), | |
| altimeter: z.string(), | |
| altitude: z.string(), | |
| temperature: z.string(), | |
| visibility: z.string(), | |
| wind_speed: z.string(), | |
| }); |
Any reason for the empty lines? Unless some linting requires this, I'd keep it compressed given they are one-liners? (I get it for others where they are more complex.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Overview
Using 3rd-party APIs in statically typed languages poses a challenge as you need some way to validate the type of the returned data to avoid type errors at runtime. So far, the codebase used TypeScript's
anytype on responses from 3rd-party APIs to access values on the returned data. Asanybasically means opting out of all type checking, it's best practice to avoid its usage wherever possible. Therefore, the upcoming lint overhaul will disallowanycompletely.Refactoring all currently used API implementations to avoid using
anyis too complex to be included in the lint overhaul.This PR is a mix of
featandrefactor: It introduces a new generic API wrapper and migrates all existing 3rd-party APIs to use it.Description
The API wrapper uses the 3rd-party library Zod to validate the returned data against statically declared schemas. It leverages TypeScript's ability to implicitly infer types thereby enabling type checking on the returned data. Code completion on the returned data is another QoL feature added by this PR/library.
Basic usage
Details
Using the wrapper is as simple as declaring a Zod schema for the returned data:
Then using the
fetchForeignAPI<ReturnType>(request, schema)method:For simplicity you can also just pass a
URLobject orstringtofetchForeignAPI().Testing
A list of all modified commands can be found below.
While testing make sure to use improbable inputs as well as more typical inputs. E.g. requesting station/taf/metar information from small airports, airports with almost no traffic etc.
Consider any case where the command doesn't succeed as 'failed'. The goal is to get correct typing, not limit the data we can show/use.
Affected Commands
/vatsim data <all_subcommands>/vatsim events/station/taf/metar/live-flights/simbrief-data retrieve/wolframalphaAdditional Information
The
/tafembed has been slightly modified to be more consistent. Instead of decoding the first part of the TAF only, it now only provides the raw string and links to our docs and https://e6bx.com/weather/{ICAO}/?showDecoded=1&focuspoint=tafdecoder which decodes the TAF for the queried airport.Discord Username
examplewastaken