feat: use discriminated JS return types (#11)

[velocitysystems]

#11

Replaces the single Download class with a discriminated union of state-specific types, providing compile-time safety for download operations.

• Invalid method calls (e.g., pause() on a created download) are caught at compile-time
• Autocomplete only shows valid methods for each state
• Method chaining is fully typed through state transitions

Updated example app to use type guards.
Updated README with new API documentation.

[jthomerson]

Some drive-by comments on this, although I'm unfamiliar with the code itself, and that will need @yokuze's review...

• I'm not sure that this needs to be four commits. Feels like it should be one; from our commit standards: "The basic principle is each commit should be for a single piece of work, well described, and not contain unrelated changes.". All four of these commits are very closely related to the same change, and "future you" will probably benefit from seeing them together in a single commit.
• There's a bunch of package-lock changes, but no changes to package.json ... so I assume those package-lock changes are errors? They look like noise, or at the very least, are an unrelated change (to use exact versions).
• Are all the is* functions necessary? If, instead, we used our Toolbox StrictUnion, would that make it so you could simply if (dl.state === SomeState) { ... right type is here }? That's more of a @yokuze question, as he's much better with TS types than I am.

[onebytegone]

• Are all the is* functions necessary? If, instead, we used our Toolbox StrictUnion, would that make it so you could simply if (dl.state === SomeState) { ... right type is here }? That's more of a @yokuze question, as he's much better with TS types than I am.

A drive-by on the drive-by 😆. Because these are discriminated unions, it doesn't look like we need StrictUnion or all the is* functions. StrictUnion is only needed when we don't have a discriminator field in the type. Playground example:

https://www.typescriptlang.org/play/?#code/FAUwdgrgtgBAIgewO5gDYIIYBMDKAXDPEGAb2BhgGEAlAUQEEAVWuGAXhgHIBjAJxEIgsnADTkYASQByAfQAK1APIBxOjhzsuASzBzeCAOb8AzsdHi59AKo4WmzgAcMEY0PMBfYMB1FeAMwxuYkp+QSxEFHRsUnEYYwIiAC54ZDRMXASQADoaBmY4AG5gT28wXwCgmHpuPC0ANxAItOiyCjjM5KaojMEs6XklVVp1IpKfEH9A4jlnV3DU7pi2+MFOhfT8XssbFlGvPABPB2Iu9PZYgB8qUKJ5yPTLqpr6xvXsR5mXIVPsIuBuBBgeIwLBvLCaEjtVYpe7YTZEHJ0Jh2dwwDDGGHNLB-bx+GAAClBsKwWRWRHYbA4Px6CP6ChUahwAEolhQAPRsmCMAAWWgxxm5CAgqHBE30vFiAKBeBgfAEt2SIXl3zBmiJWL+bQ5XN5-MFwvBACNiAgANaSwHAwK1BrJao217EtVg0ZAA

[velocitysystems]

Thanks @jthomerson, @onebytegone. I've squashed the commits, removed the unrelated package-lock changes^ and the is* functions by applying your suggested changes.

^Note: Will address the package-lock changes in a future PR to update npm/crate versions.

[yokuze]

Thanks for the IRL brainstorming session! I left some comments with a few of the things we came up with.

Here's a draft of the concepts we talked about. Feel free to use some of it or none of it, whatever makes sense:

export enum DownloadStatus {
   NotCreated = 'notCreated',
   Created = 'created',
   InProgress = 'inProgress',
   Paused = 'paused',
   Cancelled = 'cancelled',
   Completed = 'completed',
   Unknown = 'unknown',
}

export enum DownloadAction {
   Create = 'create',
   Start = 'start',
   Resume = 'resume',
   Cancel = 'cancel',
   Pause = 'pause',
   Listen = 'listen',
}

export interface DownloadState<S extends DownloadStatus> {
   key: string;
   url: string;
   path: string;
   progress: number;
   status: S;
}

export interface DownloadActionResponse<A extends DownloadAction = DownloadAction> {
   download: DownloadWithAnyStatus;
   expectedStatus: ExpectedStatusesForAction<A>;
   isExpectedStatus: boolean;
   error?: Error;
}

export interface AllDownloadActions {
   [DownloadAction.Listen]: (listener: (download: DownloadWithAnyStatus) => void) => Promise<UnlistenFn>;
   [DownloadAction.Create]: () => Promise<DownloadActionResponse<DownloadAction.Create>>;
   [DownloadAction.Start]: () => Promise<DownloadActionResponse<DownloadAction.Start>>;
   [DownloadAction.Resume]: () => Promise<DownloadActionResponse<DownloadAction.Resume>>;
   [DownloadAction.Cancel]: () => Promise<DownloadActionResponse<DownloadAction.Cancel>>;
   [DownloadAction.Pause]: () => Promise<DownloadActionResponse<DownloadAction.Pause>>;
}

// Only these actions are allowed for each given DownloadStatus:
export const allowedActions = {
   [DownloadStatus.NotCreated]: [
      DownloadAction.Create,
      DownloadAction.Start,
   ],
   [DownloadStatus.Created]: [
      DownloadAction.Listen,
      DownloadAction.Start,
      DownloadAction.Cancel,
   ],
   [DownloadStatus.InProgress]: [
      DownloadAction.Listen,
      DownloadAction.Pause,
   ],
   [DownloadStatus.Paused]: [
      DownloadAction.Listen,
      DownloadAction.Resume,
      DownloadAction.Cancel,
   ],
   [DownloadStatus.Completed]: [],
   [DownloadStatus.Cancelled]: [],
   [DownloadStatus.Unknown]: [
      DownloadAction.Listen,
   ],
} satisfies Record<DownloadStatus, DownloadAction[] | []>;

export const expectedStatusesForAction = {
   [DownloadAction.Create]: [ DownloadStatus.Created ],
   [DownloadAction.Start]: [ DownloadStatus.InProgress ],
   [DownloadAction.Cancel]: [ DownloadStatus.Cancelled ],
   [DownloadAction.Pause]: [ DownloadStatus.Paused ],
   [DownloadAction.Resume]: [ DownloadStatus.Paused ],
   // Everything but "unknown" is valid:
   [DownloadAction.Listen]: [
      DownloadStatus.Cancelled,
      DownloadStatus.Created,
      DownloadStatus.Completed,
      DownloadStatus.InProgress,
      DownloadStatus.NotCreated,
      DownloadStatus.Paused,
   ],
} satisfies Record<DownloadAction, DownloadStatus[] | []>;

type ActionsFns<S extends DownloadStatus> = Pick<AllDownloadActions, typeof allowedActions[S][number]>;
type AllowedActionsForStatus<S extends DownloadStatus> = ActionsFns<S> extends never ? object : ActionsFns<S>;

export type Download<S extends DownloadStatus> = DownloadState<S> & AllowedActionsForStatus<S>;

/**
 * Union type representing a download in any status.
 *
 * To narrow the type to a more specific Download status, use either
 * {@link hasAction `hasAction`} or the `status` field as a discriminator.
 *
 * @example
 * ```ts
 * if (hasAction(download, DownloadAction.Start)) {
 *    await download.start();
 * }
 *
 * // Or:
 * if (download.status === DownloadStatus.Created) {
 *   await download.start(); // TypeScript knows start() is available
 * }
 * ```
 */
export type DownloadWithAnyStatus = { [T in DownloadStatus]: Download<T> }[DownloadStatus];

export type ExpectedStatusesForAction<A extends DownloadAction> = (typeof expectedStatusesForAction)[A][number];
export type UnexpectedStatusesForAction<A extends DownloadAction> = Exclude<DownloadStatus, ExpectedStatusesForAction<A>>;

export type ExpectedStatesForAction<A extends DownloadAction> = Extract<DownloadWithAnyStatus, Pick<AllDownloadActions, A>>;
export type UnexpectedStatesForAction<A extends DownloadAction> = Exclude<DownloadWithAnyStatus, ExpectedStatesForAction<A>>;

type DownloadsWithAction<A extends DownloadAction> = Extract<DownloadWithAnyStatus, Pick<AllDownloadActions, A>>;

export function hasAction<A extends DownloadAction>(download: DownloadWithAnyStatus, actionName: A): download is DownloadsWithAction<A> {
   return (allowedActions[download.status] as DownloadAction[]).includes(actionName);
}

/**
 * @returns `true` if the download is in a state that does not allow any further actions,
 * i.e. it is completed or cancelled
 */
export function hasAnyValidActions(download: DownloadWithAnyStatus): download is Exclude<DownloadWithAnyStatus, DownloadStatus.Completed | DownloadStatus.Cancelled> {
   return download.status === DownloadStatus.Completed || download.status === DownloadStatus.Cancelled;
}

async function sendAction<A extends DownloadAction>(action: A, args: Record<string, unknown>): Promise<DownloadActionResponse<A>> {
   const response = await invoke<DownloadActionResponse<A>>('plugin:download|' + action, args);

   response.download = createDownload(response.download);

   return response;
}

const actions = {
   listen(listener: (download: DownloadWithAnyStatus) => void): Promise<UnlistenFn> {
      return DownloadEventManager.shared.addListener(this.key, listener);
   },

   async create() {
      return sendAction(DownloadAction.Create, { key: this.key, url: this.url, path: this.path });
   },

   async start() {
      return sendAction(DownloadAction.Start, { key: this.key });
   },

   async resume() {
      return sendAction(DownloadAction.Resume, { key: this.key });
   },

   async cancel() {
      return sendAction(DownloadAction.Cancel, { key: this.key });
   },

   async pause() {
      return sendAction(DownloadAction.Pause, { key: this.key });
   },
} satisfies AllDownloadActions & ThisType<DownloadState<DownloadStatus>>;

/**
 * Creates a Download object with the allowed actions for the given state
 *
 * @param state The download item from the plugin
 */
function createDownload<S extends DownloadStatus>(state: DownloadState<S>): Download<S> {
   const download = {
      key: state.key,
      url: state.url,
      path: state.path,
      progress: state.progress,
      status: state.status,
   } satisfies DownloadState<S>;

   const actionsForDownload = allowedActions[state.status];

   for (const actionName of actionsForDownload) {
      Object.defineProperty(download, actionName, {
         value: actions[actionName],
      });
   }

   return download as Download<S>;
}

/**
 * @returns The list of download operations
 */
export async function list(): Promise<DownloadWithAnyStatus[]> {
   return (await invoke<DownloadState<DownloadStatus>[]>('plugin:download|list'))
      .map((item) => { return createDownload(item); });
}

/**
 * @param key The download identifier
 * @returns The download operation
 */
export async function get(key: string, url: string, path: string): Promise<DownloadWithAnyStatus> {
   const download = await invoke<DownloadState<DownloadStatus> | undefined>('plugin:download|get', { key });

   if (download) {
      return createDownload(download);
   }

   return createDownload({ key, url, path, progress: 0, status: DownloadStatus.NotCreated });
}

There are a couple things there that are different from what we came up with:

1. A download in the NotCreated state, for convenience' sake, has both .create() and .start() as possible actions. The Rust backend's start handler would accept both Created and NotCreated downloads. If NotCreated, it just creates and then starts.
2. Since we want to return a NotCreated download to allow for listening for updates and taking an action later, getOrCreate is just get to avoid confusion since it would return NotCreated if the download didn't already exist

Usage would look like this:

const props = defineProps<{ download: DownloadWithAnyStatus}>();

let unlisten: UnlistenFn | undefined;

const currentDownload = ref<DownloadWithAnyStatus>(props.download),
      hasAnyActions = computed(() => { return !hasAnyValidActions(currentDownload.value); }),
      canStart = computed(() => { return hasAction(currentDownload.value, DownloadAction.Start); }),
      canCancel = computed(() => { return hasAction(currentDownload.value, DownloadAction.Cancel); }),
      canPause = computed(() => { return hasAction(currentDownload.value, DownloadAction.Pause); }),
      canResume = computed(() => { return hasAction(currentDownload.value, DownloadAction.Pause); });

onMounted(listenToEvents);
onUnmounted(() => { return unlisten?.(); });

async function listenToEvents(): Promise<void> {
   if (unlisten || !hasAction(currentDownload.value, DownloadAction.Listen)) {
      return;
   }
   unlisten = await currentDownload.value.listen((updated) => {
      currentDownload.value = updated;
   });
}

function onError(error: Error): void {
   console.error(error);
}

type StatusHandlers<A extends DownloadAction> = Partial<{
   [S in UnexpectedStatusesForAction<A>]: (actualState: Download<S>) => void;
}>;

type ActionHandlers = Partial<{
   [K in DownloadAction]: StatusHandlers<K>;
}>;

const unexpectedStatusHandlers: ActionHandlers = {
   [DownloadAction.Start]: {
      [DownloadStatus.Cancelled]: () => {
         // Tried to start the download but it was cancelled instead
      },
   },
   [DownloadAction.Resume]: {
      [DownloadStatus.Cancelled]: () => {
         // Tried to start the download but it was cancelled instead
      },
   },

   [DownloadAction.Cancel]: {
      [DownloadStatus.Completed]: (): void => {
         // You'll probably want to delete the file since the user wanted to cancel
         // the download but wasn't able to before it completed
      },
      [DownloadStatus.InProgress]: (): void => {
         // There was a problem cancelling the download
      },
   },

   [DownloadAction.Pause]: {
      [DownloadStatus.InProgress]: (): void => {
         // There was a problem pausing the download
      },
      [DownloadStatus.Completed]: (): void => {
         // The user tried to pause a completed download. This probably doesn't matter as
         // much as the other cases
      },
   },
};


function handleUnexpectedStatus(action: DownloadAction, result: DownloadActionResponse<DownloadAction>): void {
   const handlers = action in unexpectedStatusHandlers ? unexpectedStatusHandlers[action] : undefined;

   if (!handlers) {
      return;
   }

   const download = result.download,
         status = download.status as keyof Required<typeof handlers>;

   if (download.status === status && handlers[status]) {
      handlers[status](download);
   }
}

async function doAction<A extends Exclude<DownloadAction, DownloadAction.Listen>>(action: A): Promise<void> {
   if (hasAction(currentDownload.value, action)) {
      const result = await currentDownload.value[action]();

      if (result.error) {
         onError(result.error);
      } else if (!result.isExpectedStatus) {
         handleUnexpectedStatus(action, result as DownloadActionResponse<A>);
      }
   }
}

[yokuze]

Needs a trailing comma ,

[yokuze]

These enum keys should be PascaleCase: https://github.com/silvermine/eslint-config-silvermine/blob/master/partials/typescript.js#L99

This and the lines below it should be indented 3 spaces

[yokuze]

Something is amiss with the ESLint config: it doesn't seem to be applying to this file. There are a few violations that should've been caught (outlined in comments below).

[yokuze]

This and the lines below it should be indented 3 spaces

[yokuze]

We have: DownloadState, DownloadItem, and Download as user-facing types. I wonder if we could clarify their purpose with the names:

• DownloadStatus formerly enum DownloadState - The download's current status. This implies that DownloadItem's state field would become status
• DownloadState formerly interface DownloadItem - an immutable, plain object that represents the complete serializable state of the download that comes over the IPC
• Download - Stays as-is. Conceptually it's DownloadState + methods to take action on it (e.g. pause, play)

What do you think?

[yokuze]

ADIRL: do not allow listen for downloads that will never have updates, such as as those with status Cancelled and Completed

[yokuze]

Since this represents an in-progress download and the status is also called IN_PROGRESS, should we call this InProgressDownload? Mostly for consistency's sake, but also "Active" could be taken to mean a download object that's able to be interacted with, e.g. by calling "start" on a paused download.

[yokuze]

ADIRL: The IPC will respond with some kind of response objects with a few different helpful properties. We would return that response object for each of these methods.

One of the response object's properties would be a Download object that represents the actual state at the time the Rust layer sends its response. In most cases, this would be a download object with the state you'd expect given the action you just took, e.g. for the resume method a Download object with status of InProgress. But it also allows us to return some other state in the case when the action we tried to take doesn't match up with the actual backend-state of the download, e.g. sending a "pause" action when the download is in a "Completed" state.

[yokuze]

These classes work and are fairly straightforward, but they have a couple disadvantages:

1. CancelledDownload, CompletedDownload, and UnknownDownload don't do anything other than change what state is initialized to.
2. Classes that have the same methods, e.g. PausedDownload#cancel, ActiveDownload#cancel, CreatedDownload#cancel implement the cancel method individually, 3 times. Granted, the implementation is pretty simple, but not-DRYing has the risk of diverging implementations over time

(See the code sample I left for one suggestion)