facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.86k forks source link

Suggestion: Immutable Mode #5369

Open ajhyndman opened 6 years ago

ajhyndman commented 6 years ago

I've noticed a trend in issues I have observed over the past year, as I've been actively using Flow: A large proportion of the confusing and unpredictable aspects of Flow ultimately stem from the mutable, dynamic nature of JavaScript.

Flow has adopted the philosophy that the dynamic, mutable nature of JS should be embraced and type soundness in that world is both achievable and desirable. I understand and respect that (as a means of allowing incremental adoption, if nothing else).

This means that to support read-only data and types, Flow has needed to introduce explicit opt-in syntax via helpers like $ReadOnly<>, $ReadOnlyArray<>, exact object syntax, and explicit covariant property definitions. These are also great additions to Flow, and I'm excited to have them!

However, in our codebase, we are finding that larger and larger portions of our code want to opt in to the immutability assertions. This means most of our code is starting to look like this:

export type Props = {|
  +block?: boolean,
  +disabled?: boolean,
  +onChange: (newValue?: string) => void,
  +options: $ReadOnlyArray<string>,
  +placeholder?: string,
  +selected?: string,
|};

export type State = {|
  +isFocused: boolean,
  +activeIndex: ?number,
  +width?: number,
|};

We are finding that it is altogether too easy to forget to use covariance, exact object notation, or a $ReadOnly assertion. In addition, our type signatures are starting to look scarier for new developers joining the team.

I would like to suggest a way out of this might be to include a file or project-level configuration option which allows developers to enforce immutable, read-only usage of data within that scope. I think this might improve Developer Experience while using Flow in projects that prefer to make the stronger read-only assertions by default.

In addition, I suspect a read-only project or file-level assertion might allow Flow to make more useful, stronger assumptions in the future. For instance, perhaps flow could immediately identify any mutative call to [...].sort() as an error (or lint warning).

I'd be keen to hear ideas for any other solutions that might help address this DX issue, as well!

Andarist commented 6 years ago

Yeah, that would be super useful. We are finding ourselves using $Exact, {||} and $ReadOnly all over the place and it introduces quite a noise.

bradennapier commented 6 years ago

I also ran into this and ended up with a styling using utility types so that I can compose the objects yet still use them as shapes, etc when building them.

Not ideal but it does begin to provide a nicer looking overall file while maintaining the strictness of the types.

Using immutability everywhere runs into its own issues when you want to use parts of that type without re-writing it multiple times and doing a $Shape<> of a strict immutable object can run into issues and errors. So this method maintains both in a way that is pretty easy to work with.

type ObjShape = {
   one: string
}

type ObjStrict = $StrictObject<MyObject>
type AsNonMaybeObject = <O>(O) => $ObjMap<O, <T>(T) => $NonMaybeType<T>>;
type AsStrictObject = <O>(O) => $ExactReadOnly<$NonMaybeObject<O>>;

/*
  { one: void | string, two: void | number, three?: string } -->
  {| +one: void | string, +two: void | number, +three?: string |}
*/
declare type $ExactReadOnly<O> = $ReadOnly<$Exact<O>>;

/*
  { one: void | string, two: void | number, three?: string } -->
  { one: string, two: number, three?: string }
*/
declare type $NonMaybeObject<O> = $Call<AsNonMaybeObject, O>;

/*
  { one: void | string, two: void | number, three?: string } -->
  {| +one: string, +two: number, +three?: string |}
*/
declare type $StrictObject<O> = $Call<AsStrictObject, O>;

/*
  { one: string, two: number, three?: string } -->
  { +one?: string, +two?: number, +three?: string }
*/
declare type $ReadOnlyShape<O> = $ReadOnly<$Shape<O>>;

/*
  string -->
  Array<string> | string
*/
declare type $TypeOrArray<T> = Array<T> | T;

/*
  Array<string> | string -->
  string
*/
declare type $NonArrayType<A> = $Call<(<T>(Array<T> | T) => T), A>
sharno commented 6 years ago

I feel like making the exact object literal {||} as default would make perfect sense because it enforces people to use objects only as records instead of dealing with it as a Map. Also, making immutability the default could have the same argument of enforcing a good practice. But not sure if Flow is targeting backward compatibility or if they allow big changes until version 1?

It seems the easy way is to put that into config (which I hope to be the default because I love good defaults)

Peeja commented 6 years ago

@sharno Can you clarify what you mean? Do you mean that { foo: string } would be an exact type and you'd need to use additional syntax to make it not exact?

Exactness is really orthogonal to what this issue is about. This is about immutability, meaning $ReadOnly<>. You can have only immutable objects and still take advantage of width subtyping, by passing an object whose properties you know a lot about to a function which only cares about some of those properties.

sharno commented 6 years ago

@Peeja Yeah that's a good point, I just thought it'd be a good idea to include exactness in the discussion. I just have used exactness so much more than regular object in the code that I write and noticed that ReasonML/OCaml has this separation between records (exact objects) and maps, so thought it's a good idea.

ajhyndman commented 6 years ago

@sharno I was also imagining that exact might be a nicer default with extra syntax to opt-out.

But I think @Peeja is probably right, it's not a requirement to make an immutable mode useful, and requiring new syntax might be enough of a strike against it to outweigh the value.

sam-n-johnston commented 6 years ago

I had a similar problem where I wanted to prevent Redux state & React component props to be mutated without the use of extra libraries like Immutable which requires learning the API for newcomers. This is how we our solving it in our codebase and it works pretty well until now! I would love to have a built-in one in Flow though.

This would simplify greatly your definitions:

export type Props = RecursiveReadOnly<{|
  block?: boolean,
  disabled?: boolean,
  onChange: (newValue?: string) => void,
  options1: string[], // No need to remember to put it here
  options2: { notMutable: { stilNotMutable: number[] }[] }[], // Nice try, can't mutate this either!
  placeholder?: string,
  selected?: string,
|}>;

I have been trying to figure out how to do the reverse (RecursiveReadAndWrite), which would take a deeply read-only object and return a read-and-write object. This is what is returned by deep copy libraries for example, but I have not been successful. But I think, in our case, our need for this is mostly because of a bad design in a single place in our codebase.

mrkev commented 6 years ago

Big fan of this, would love to see it happen. I can think of two ways it might.

As an extension to the @flow directive, say @flow immutable.

This could by default make all types defined in the file be immutable, and go further in enforcing some extra assertions regarding what functions in the file can do (aka, it could enforce functions in the file to be pure). Much like @flow strict this should require all dependencies are also immutable, so as to not break any assumptions.

As a tag on the type, like %checks for functions. Call it %immutable

Usage would look as follows:

export type Props = {
  block?: boolean,
  disabled?: boolean,
  onChange: (newValue?: string) => void,
  options: $ReadOnlyArray<string>,
  placeholder?: string,
  selected?: string,
} %immutable;

This would probably need to be shallow --options in the case above would still need to be defined as a $ReadOnlyArray-- so as to not force possibly confusing semantic changes to user-defined types and maintain correctness like say:

type Foo = { bar: number } // Not exact or read-only

type Baz = { foo: Foo } %immutable

const f = (foo: Foo): Baz => ... // the foo coming in is mutable. Correctly making the one in Baz immutable would be hard to get right and confusing if done so.

Linking to this thread, for the sake of cross-reference: https://github.com/facebook/flow/issues/2715

AMongeMoreno commented 4 years ago

Is there any plans on including this at some point? Many modern projects and developers have interiorized the benefits of using immutable objects, with many libraries helping you ensure this in runtime which I always felt like a waste of computation effort. Having an option to enable 'immutable' by default from the type checker for all user defined types would make everything so much easier, and I also think it would help flow reason faster as it would be able to infer much easier.