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

Accept readonly array inputs for all functions/methods that don't need to mutate their data #5831

Open
lionel-rowe opened this issue Aug 26, 2024 · 1 comment
Labels
good first issue Good for newcomers PR welcome A pull request for this issue would be welcome

Comments

@lionel-rowe
Copy link
Contributor

lionel-rowe commented Aug 26, 2024

Further to #5734

Is your feature request related to a problem? Please describe.

Currently, passing data marked as const to various @std functions/methods fails type checking, even though the passed data is never modified by the function in question.

For example, msgpack/encode:

import { encode, type ValueType } from "jsr:@std/msgpack/encode";

const data = {
  a: 1,
  b: { c: 2n },
  d: [3, { e: 4 }],
} as const;

// Argument of type '{ readonly a: 1; readonly b: { readonly c: 2; }; readonly d: readonly [3, { readonly e: 4; }]; }' is not assignable to parameter of type 'ValueType'.
//   Type '{ readonly a: 1; readonly b: { readonly c: 2; }; readonly d: readonly [3, { readonly e: 4; }]; }' is not assignable to type 'ValueMap'.
//     Property 'd' is incompatible with index signature.
//       Type 'readonly [3, { readonly e: 4; }]' is not assignable to type 'ValueType'.
//         Type 'readonly [3, { readonly e: 4; }]' is not assignable to type 'ValueMap'.
//           Index signature for type 'string' is missing in type 'readonly [3, { readonly e: 4; }]'.deno-ts(2345)
encode(data);

Describe the solution you'd like

Mark all array arguments as readonly unless they are mutated by the function in question. The readonly-ness then becomes explicitly part of the contract of the function, hence changing the implementation such that it mutates such arrays would be a breaking change (typically it'd already be a breaking change though, given that consumers typically don't expect functions to mutate their inputs unless such mutation is explicitly the purpose of the function).

Describe alternatives you've considered

Using the same example data as above:

// weirdly still fails type checking
encode(structuredClone(data));

// throws due to bigint
encode(JSON.parse(JSON.stringify(data)));

// passes type checking but loses all type safety and fails `deno-lint(no-explicit-any)`
encode(data as any);

// eww
encode(data as unknown as ValueType);

// passes but verbose/brittle and incurs a runtime performance cost
encode({ ...data, d: [...data.d] });

// probably easy enough to create a `Mutable` utility type that'd work here,
// but shouldn't be necessary given the data is never mutated
encode(data as Mutable<typeof data>);
@iuioiua
Copy link
Collaborator

iuioiua commented Aug 26, 2024

This seems like good practice. +1 from me.

@iuioiua iuioiua added feedback welcome We want community's feedback on this issue or PR good first issue Good for newcomers PR welcome A pull request for this issue would be welcome and removed feedback welcome We want community's feedback on this issue or PR labels Aug 26, 2024
iuioiua pushed a commit that referenced this issue Aug 27, 2024
feat(msgpack): accept readonly input data (#5831)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers PR welcome A pull request for this issue would be welcome
Projects
None yet
Development

No branches or pull requests

2 participants