Do you want to work on this issue?

You can request for a bounty in order to promote it!

Support $ReadOnlyArray #199

christophehurpeau posted onGitHub

This is a:

Bug Report
Feature Request
Question
Other

Which concerns:

flow-runtime
babel-plugin-flow-runtime
flow-runtime-validators
flow-runtime-mobx
flow-config-parser
The documentation website

type Thing = $ReadOnlyArray<string | number>;

const thing: Thing = [];

console.log(thing);

Output:

Thing must be an object

Expected: {
  [Symbol(Symbol.iterator)]: () => Iterator<T>;
  toLocaleString: () => string;
  concat: <S, Item: $ReadOnlyArray<S> | S> (...items: Array<Item>) => Array<T | S>;
  entries: () => Iterator<[number, T]>;
  every: (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => boolean;
  filter: ((callbackfn: Boolean) => Array<$NonMaybeType<T>>) | ((callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => Array<T>);
  find: (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => T | void;
  findIndex: (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => number;
  forEach: (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => void;
  includes: (searchElement: mixed, fromIndex?: number) => boolean;
  indexOf: (searchElement: mixed, fromIndex?: number) => number;
  join: (separator?: string) => string;
  keys: () => Iterator<number>;
  lastIndexOf: (searchElement: mixed, fromIndex?: number) => number;
  map: <U> (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => U, thisArg?: any) => Array<U>;
  reduce: ((callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: $ReadOnlyArray<T>) => T, initialValue: void) => T) | (<U> (callbackfn: (previousValue: U, currentValue: T, currentIndex: number, array: $ReadOnlyArray<T>) => U, initialValue: U) => U);
  reduceRight: ((callbackfn: (previousValue: T, currentValue: T, currentIndex: number, array: $ReadOnlyArray<T>) => T, initialValue: void) => T) | (<U> (callbackfn: (previousValue: U, currentValue: T, currentIndex: number, array: $ReadOnlyArray<T>) => U, initialValue: U) => U);
  slice: (start?: number, end?: number) => Array<T>;
  some: (callbackfn: (value: T, index: number, array: $ReadOnlyArray<T>) => any, thisArg?: any) => boolean;
  values: () => Iterator<T>;
  length: number;
  [key: number]: T;
}

Actual Value: []

Actual Type: Array<any>

no errors with flow

@issuehunt has funded $40.00 to this issue.

Submit pull request via IssueHunt to receive this reward.
Want to contribute? Chip in to this issue via IssueHunt.
Checkout the IssueHunt Issue Explorer to see more funded issues.
Need help from developers? Add your repository on IssueHunt to raise funds.

posted by issuehunt-app[bot] over 5 years ago

@gajus any ideas how can covariance be supported in general? I can only think of Object.freeze, but not sure if validators are allowed to do something like this to original values

posted by goodmind over 5 years ago

I don't see a particular reason not to use Object.freeze. Since expectation is to prevent modification of array, this seems like a reasonable solution.

posted by gajus over 5 years ago

@gajus most babel-plugin-flow-runtime assertions throw errors though; I think analogous support for $ReadOnlyArray would wrap the runtime Array in a Proxy that throws if any modification is attempted, rather than just silently preventing modification; do you agree?

EDIT sorry, I forgot that Object.freeze causes errors to be thrown if modification is attempted.

(In any case Object.freeze is better than nothing)

posted by jedwards1211 over 5 years ago

@gajus actually I think there's a problem. Say we pass an array from a context where it's intended to be writable to a context where it's intended to be read-only. Object.freeze would prevent writes in the context where they're intended to work.

Dumb example but:

function logArray(arr: $ReadOnlyArray<number>): Array<number> {
  console.log(arr)
}

const arr: Array<number> = [1, 2, 3]
logArray(arr)
arr.push(4) // whoops, throws if babel-plugin-flow-runtime is applied, due to Object.freeze?
logArray(arr)

posted by jedwards1211 over 5 years ago

I commented in the PR but I think making babel-plugin-flow-runtime wrap the array with a proxy is the only solution that avoids the above problem. A lot more work to wrap the array in all cases $ReadOnlyArray can be used though

posted by jedwards1211 over 5 years ago

@gajus actually I think there's a problem. Say we pass an array from a context where it's intended to be writable to a context where it's intended to be read-only. Object.freeze would prevent writes in the context where they're intended to work.

Why not just make a copy before object freeze?

const newArray = Object.freeze([...input]);

Then the original parameter is not impacted. And since the intention is to have it as read-only, seems reasonable to assume that modifying the array should not affect wherever it came from.

posted by gajus over 5 years ago

Problematic code

const foo: $ReadOnlyArray<number> = [1, 2, 3]
const bar: Array<string> = (foo: any); // bypassing typecheck, but array is still frozen

We can store original array alongside frozen one probably? Or make it work like function or class annotations

posted by goodmind over 5 years ago

I mean... any is um... you are shooting yourself in the foot anyway.

User can easily work around this with:

const foo: $ReadOnlyArray<number> = [1, 2, 3]
const bar: Array<string> = [
  ...foo
];

posted by gajus over 5 years ago

I thought that any removes any type-assertions, but checked on site, this is not the case with functions or any other type

posted by goodmind over 5 years ago

I think that supporting any is a separate issue altogether. We could (as you mentioned) keep a copy of whatever the input is and revert it back to the original in case of any. I think that supporting any is pushing it to edge cases though.

posted by gajus over 5 years ago

@gajus there's also warning mode which wouldn't work with Object.freeze, it would always throw

posted by goodmind over 5 years ago

@gajus there's also warning mode which wouldn't work with Object.freeze, it would always throw

A sensitive thing to do would be to disable this in warning mode.

posted by gajus over 5 years ago

@gajus careful, copying the array causes problems too. $ReadOnlyArray is just a read-only "view", Flow doesn't prevent the view from seeing changes to the array made externally later on, and I don't think it's safe for flow-runtime to do so either. Instead of copying it why not just wrap it in a proxy that blocks write operations?

posted by jedwards1211 over 5 years ago

Instead of copying it why not just wrap it in a proxy that blocks write operations?

My beef with wrapping it in proxy is that (correct me if I am wrong) but the Array.isArray check will fail.

posted by gajus over 5 years ago

That's probably true, actually a problem with proxy or copy approach is that neither is === the original array

posted by jedwards1211 over 5 years ago

I think the only 100% solid approach is for the Babel plugin to add code to any index assignments, length assignments, or function calls on the $ReadOnlyArray identifier

posted by jedwards1211 over 5 years ago

Fund this Issue

$40.00

Funded