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

feat: generic wrapper for JSON APIs #70

Draft
wants to merge 36 commits into
base: staging
Choose a base branch
from

Conversation

ExampleWasTaken
Copy link
Contributor

@ExampleWasTaken ExampleWasTaken commented Jul 4, 2024

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 any type on responses from 3rd-party APIs to access values on the returned data. As any basically means opting out of all type checking, it's best practice to avoid its usage wherever possible. Therefore, the upcoming lint overhaul will disallow any completely.

Refactoring all currently used API implementations to avoid using any is too complex to be included in the lint overhaul.
This PR is a mix of feat and refactor: 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:

import { z } from 'zod';

export const PersonSchema = z.object({
  name: z.string(),
  age: z.number(),
});

export type Person = z.infer<typeof PersonSchema>;

Then using the fetchForeignAPI<ReturnType>(request, schema) method:

import { Request } from 'node-fetch';

try {
  const data = await fetchForeignAPI(new Request('https://api.example.com/'), PersonSchema);

  data.[name, age] // code completion and type checking
} catch (e) {
  if (e instanceof ZodError) {
    // full access to the error - note that fetchForeignAPI() already logs the error to the console as well as the endpoint from which the data was retrieved
  }
}

For simplicity you can also just pass a URL object or string to fetchForeignAPI().

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

Command Status
/vatsim data <all_subcommands>
/vatsim events image
/station image
/taf image
/metar image
/live-flights image
/simbrief-data retrieve image
/wolframalpha image

Additional Information

The /taf embed 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

@ExampleWasTaken ExampleWasTaken marked this pull request as ready for review July 4, 2024 01:57
@pdellaert pdellaert self-assigned this Jul 11, 2024
@pdellaert
Copy link
Collaborator

I like this in general, but needs a deeper review than I can do right now :)

Copy link
Member

@benw202 benw202 left a comment

Choose a reason for hiding this comment

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

LGTM, will wait on a second review from @pdellaert on this one

@ExampleWasTaken ExampleWasTaken marked this pull request as draft August 22, 2024 08:35
@ExampleWasTaken ExampleWasTaken marked this pull request as ready for review August 26, 2024 14:45
Copy link
Collaborator

@pdellaert pdellaert left a comment

Choose a reason for hiding this comment

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

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(),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@benw202 benw202 marked this pull request as draft November 11, 2024 16:17
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.

4 participants