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

Take a readonly array when possible #1314

Closed
3 tasks done
bengry opened this issue Jun 11, 2019 · 6 comments · Fixed by #2059
Closed
3 tasks done

Take a readonly array when possible #1314

bengry opened this issue Jun 11, 2019 · 6 comments · Fixed by #2059
Labels
enhancement Possible enhancement Typescript Issue related to Typescript typings

Comments

@bengry
Copy link

bengry commented Jun 11, 2019

Bug report

  • I've checked documentation and searched for existing issues
  • I've made sure my project is based on the latest MST version
  • Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code

type TodoType = "task" | "bug";
// written as a separate variable for other references that are not tied to MST
const todoTypes: ReadonlyArray<TodoType> = ["task", "bug"];
const todoTypes: ["task", "bug"] as const // same as above, but shorter, for TS >= 3.4

export const Todo = types
  .model("Todo", {
    id: types.optional(types.number, () => Math.random()),
    title: types.string,
    finished: false,
    type: types.enumeration(todoTypes) // error
  })

Describe the expected behavior
The above code should compile

Describe the observed behavior
Line 10 shows the following error:

Argument of type 'ReadonlyArray' is not assignable to parameter of type 'TodoType[]'.
Type 'ReadonlyArray' is missing the following properties from type 'TodoType[]': pop, push, reverse, shift, and 6 more

This is due to enumeration accepting T[], and not a ReadonlyArray<T>/readonly T[] (latter as of TS 3.4).

This behavior is not just for this case, but also in other places in MST (e.g. types.union.

My suggestion is to accept the minimal necessary interface for arrays when possible. This should cover most, if not all cases, since they are usually not directly used anyway, at least for types. This is more important with TypeScript 3.4+ since it's now much less of a hassle to make an array a ReadonlyArray, especially with as const, but also with the readonly modifier added to more use-uses (e.g. function parameters).

If this is handled, upgrading to TypeScript 3.4 (or greater) should be considered, since it makes the changes easier to type. e.g.:

// TS 3.3
export function enumeration<T extends string>(options: ReadonlyArray<T>): ISimpleType<UnionStringArray<T[]>>
export function enumeration<T extends string>(
    name: string,
    options: ReadonlyArray<T>
): ISimpleType<UnionStringArray<T[]>>

// TS 3.4
export function enumeration<T extends string>(options: readonly T[]): ISimpleType<UnionStringArray<T[]>>
export function enumeration<T extends string>(
    name: string,
    options: readonly T[]
): ISimpleType<UnionStringArray<T[]>>

At the cost of supporting only TS 3.4+ (maybe we can transpile these to ReadonlyArray in the output type declarations?)

@xaviergonz
Copy link
Contributor

I usually use this pattern in ts with enumerations

enum Color {
  Red = "red",
  Green = "green"
}

const colorType = types.enumeration<Color>(Object.values(Color))

const myModel = types.model({
  color: colorType,
})

@xaviergonz
Copy link
Contributor

But that being said, yeah, it should take a ReadonlyArray to not leave old ts users out

@xaviergonz xaviergonz added enhancement Possible enhancement Typescript Issue related to Typescript typings labels Jun 12, 2019
@mbest
Copy link
Contributor

mbest commented Apr 22, 2020

When initializing a model with arrays, it also doesn't accept read-only arrays. Is that because the array instance is reused?

@mweststrate
Copy link
Member

mweststrate commented Apr 22, 2020 via email

@evelant
Copy link

evelant commented Feb 20, 2021

Here's another (AFAIK most DRY) way to use your readonly arrays for union types and as input for an enumeration type, just use array spread:

const myThings = ["foo", "bar"] as const
type MyThingsType = typeof myThings[number] // "foo" | "bar" 

types.model({
  //array spread makes a non-readonly copy so types.enumeration is happy
   myThingsEnum: types.enumeration([...myThings]) 
})

@jamonholmgren
Copy link
Collaborator

New PR created to fix the types for this: #2059.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Possible enhancement Typescript Issue related to Typescript typings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants