ZINC-FYP-2022-23 / console

ZINC UI for teaching assistants
1 stars 0 forks source link

refactor(gui): convert null to undefined when parsing YAML #43

Closed AnsonH closed 1 year ago

AnsonH commented 1 year ago

Problem

Using undefined

In TypeScript, we model nullable fields in the config YAML using undefined. For example:

export interface FileStructureValidation {
  ignore_in_submission?: string[];
  // We don't write something like:
  // ignore_in_submission: string[] | null;
}

A huge benefit of using ?: to mark a field as nullish is that we do not need to insert that key in an object. For instance:

interface ManyNullish {
  foo?: string;
  bar?: string;
  baz?: string;
}

const manyNullish: ManyNullish = {}; // OK to omit fields that unions with `undefined`

If we're to use null, it becomes very verbose:

interface ManyNullish {
  foo: string | null;
  bar: string | null;
  baz: string | null;
}

// 😥 We must write `XXX: null`, which is very verbose!
const manyNullish: ManyNullish = {
  foo: null;
  bar: null;
  baz: null;
}

Pitfall

However, we specify nullish values in YAML with null instead of undefined. For example:

fileStructureValidation:
  ignore_in_submission: null

Therefore, in the actual data returned by the GraphQL, nullish values equal null.

{
  /* ... */
  fileStructureValidation: {
    ignore_in_submission: null,
  }
}

This is problematic because our TypeScript type definitions do not reflect the actual runtime value. This would lead into pitfalls such as:

function foo(fsv: FileStructureValidation) {
  // 👇 Since `ignore_in_submission` equals `null` for nullish values and it will never
  // equal to `undefined` in runtime, this condition never evaluates to true in runtime!
  if (fsv.ignore_in_submission === undefined) {
    // ...
  }
}

Solution

Hence, we should convert all null to undefined when parsing the config YAML.

Note that in configToYaml(), it already converts all undefined fields to null because the YAML parser does not understand undefined.