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

chore: update generated array type definitions in TypeScript to be readonly #12947

Merged
merged 11 commits into from
Apr 15, 2024

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 11, 2024

No issue to link – this was a pain point when trying to bring the Coder generated types into Backstage as an SDK (similar to how the VS Code extension is doing things).

Changes

  • Updates type-generation logic on the Go side to make all TypeScript arrays be readonly (see notes)
  • Updates all UI code in TypeScript to remove any mutable array types, and remove accidental runtime mutations

Notes

Before, we had types like this:

interface Value {
  readonly workspaces: Workspace[];
}

With this, type Value's workspaces property can't be reassigned, but the underlying array is still fully mutable. TypeScript is perfectly happy with this:

function doStuff (value: Value) {
  const newWorkspace = makeWorkspace();
  value.workspaces.push(newWorkspace);
}

With the update, the type definitions now look like this:

interface Value {
  readonly workspaces: readonly Workspace[];
}

Both the property on the object, and the array the property points to are readonly

function doStuff (value: Value) {
  const newWorkspace = makeWorkspace();
  
  // TypeScript compiler complains
  value.workspaces.push(newWorkspace);
  
  // Perfectly fine
  const workspacesCopy = [...value.workspaces];
  workspacesCopy.push(newWorkspace);
}

Especially since both React and React Query are built around the assumption that 99% of their data is fully immutable, accidental mutations have a risk of introducing bugs that would be incredibly hard to track down

@Emyrk Emyrk marked this pull request as draft April 11, 2024 21:10
@Parkreiner Parkreiner changed the title chore: types generated handling readonly slices chore: update generated array type definitions in TypeScript to be readonly Apr 12, 2024
Comment on lines +345 to +346
{proxyContextValue.proxies &&
[...proxyContextValue.proxies]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only change – check that proxies is defined, and then if it is, spread it into a new array, and mutate that instead of the original

All the other code is exactly the same

@Parkreiner Parkreiner marked this pull request as ready for review April 12, 2024 19:03
}

// From codersdk/genericslice.go
export interface Foo<R extends any> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not for the future: R extends any actually doesn't do anything at all. Since any is a top type, everything in TypeScript extends any, so the type constraint provides zero protection

At some point, we'd probably want to turn this into

export interface Foo<R = unknown> {
  // Stuff
}

Copy link
Contributor

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Double-checked everything, as well as the story changes, and nothing seems to have gone awry.

My main worry at first was that removing some of the accidental mutations would break things, but we just happened to luck into the mutations causing really small, imperceivable changes. Definitely no-nos as far as React best practices, but we cut them off before they could do anything really nasty

@Parkreiner Parkreiner merged commit d9da054 into main Apr 15, 2024
27 checks passed
@Parkreiner Parkreiner deleted the stevenmasley/readonly_gen branch April 15, 2024 13:46
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants