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

"non-string literal property keys not supported" #380

Closed
redchair123 opened this issue Apr 13, 2015 · 35 comments
Closed

"non-string literal property keys not supported" #380

redchair123 opened this issue Apr 13, 2015 · 35 comments
Labels
Has PR incompleteness Something is missing

Comments

@redchair123
Copy link

redchair123 commented Apr 13, 2015

var x = {
    1234:"a",
    5678:"b"
}

Flow generates an error per integer key.

According to 11.1.5 of ECMA-262 this is a perfectly valid construct.

EDIT: @jgrund 's solution in #380 (comment) works well

@fatboysuns
Copy link

I get the same error when I when I run it on a jquery javascript.
$.ajax({
url: url,
dataType: 'html',
success: function(data) {
//do something
},
error: function(data) {
// something else
},
// This is the line Flow complains
statusCode: {
404: function() {
alert('404');
}
}
});

@gabelevi
Copy link
Contributor

Number literals are annoying, since we need to canonicalize them. We should probably just bite the bullet and implement the algorithm. CC @mroch

@julianacipa
Copy link

Any update on this issue?

@Zaba999
Copy link

Zaba999 commented Jan 25, 2016

👍 Is there any progress with this issue? I really need this one :)

@jgrund
Copy link
Contributor

jgrund commented Jun 7, 2016

As a really awful hack:

var x = {
    [1234]:"a",
    [5678]:"b"
}

@rosskevin
Copy link

ugh. I too need this.

@StreetStrider
Copy link

The issue still bothers. Tried to typecast to something like {[n: number]: mixed} — not working. Even if it did, it's such a crazy workaround for such a small thing.

@redchair123
Copy link
Author

Any progress?

@vkurchatkin vkurchatkin added the incompleteness Something is missing label Nov 9, 2016
@punmechanic
Copy link

punmechanic commented Nov 18, 2016

Playing devils advocate here: All keys on an object will be implicitly converted to a string. I like that this error is explicit about that behaviour (even if the error is not very well defined).

EDIT: 2016 me was an idiot and incorrect. This behaviour is surprising and unintuitive

SheetJSDev added a commit to SheetJS/sheetjs that referenced this issue Feb 3, 2017
- README and example cleanup
- basic XLSB and ODS write support
- flow typecheck for ODS file
  Note: xlsx.js flow fails: facebook/flow#380
- exposed jszip compression (fixes #220, closes #284)

README issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| #202 | @sao93859      | closes #202                                  |
| #211 | @alexanderchan | closes #211 corrected examples               |
| #327 | @cskaandorp    | changed saveAs example to match write tests  |
| #424 | @dskrvk        | added note about s2roa h/t @LeonardoPatignio |
| #496 | @jimmywarting  | closes #496 adapted rABS examples with rAAS  |

ODS file format issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| #148 | @user4815162342| closes #148 h/t @ziacik                      |
| #166 | @paulproteus   | closes #166 rudimentary ODS write support    |
| #177 | @ziacik        | closes #177                                  |
| #179 | @ziacik        | closes #179 use JSON when available          |
| #317 | @ziacik        | closes #317                                  |
| #328 | @think01       | closes #328                                  |
| #383 | @mdamt         | closes #383 duplicate cells should be copied |
| #430 | @RB-Lab        | closes #430                                  |
| #546 | @lgodard       | closes #546 thanks to other changes          |
@lenovouser
Copy link

Any plans to fix this? This should't throw an error

@arhtudormorar
Copy link

arhtudormorar commented Mar 2, 2017

This is working for me:

type PropsType = {
	[key: string]: string
}

let currencyId = 1,
    currencyCode = "RON";

let currencies: PropsType = {
	[currencyId.toString()]: currencyCode
}

console.log(currencies[1]);

@redchair123
Copy link
Author

We've internally decided to move away from flow. @jgrund 's array hack works well

@StreetStrider
Copy link

@Niggler, please, reopen the ticket. There're plenty of people who still interested in this. Let maintainers see the interest.

@redchair123 redchair123 reopened this Apr 1, 2017
@redchair123
Copy link
Author

@StreetStrider it's nearly 2 years since this issue was first raised. Contributor first commented 22 months ago, so there's definitely nonzero awareness of the issue. But at this point, you're better off switching to another typed JS alternative

@dszakallas
Copy link

dszakallas commented Apr 2, 2017

@Niggler if this were valid:

type Obj = {
  '0': number,
  56e-13214125: string
};

would you expect

const x: Obj = { '0': 0, 56e-13214125: 'hello' };
(x['0']: number);

to pass or fail?

Note, that

> { '0': 0, 56e-13214125: 'hello' }
{ '0': 'hello' }

@ndbroadbent
Copy link

@szdavid92 I would expect Flow to just understand that all object keys are strings.

a = {
  56e-13214125: 'abc'
}
// => Object {0: "abc"}

a[0]
// => "abc"

a['0']
// => "abc"

That seems pretty straightforward to me. Flow should just understand that { 0: 'abc' } is an object with a type of { string: string }. If you need integers as keys, then you have to use a Map (or an array).

I can see how this might trip up some people who are new to JS, but I think there should be an option to disable this warning when you are familiar with JS objects.

For now, I've disabled my quote-props rule in eslint, but I would love to turn that back on and disable the warning in Flow.

@itsdouges
Copy link

this would be fantastic if it was supported.

@bradennapier
Copy link

Still not supported?

@redchair123
Copy link
Author

@szdavid92 @ndbroadbent your points are well taken, but it's clear this issue isn't a priority for flow devs. In that situation it's better to either move away from flow or find a workaround. The array trick works, and combined with the comment form you can do the trick without affecting the resulting JS code:

var x = {
    /*::[*/ 1234 /*::]*/:"a",
    /*::[*/ 5678 /*::]*/:"b"
}

@chyzwar
Copy link

chyzwar commented Oct 23, 2017

If there is a fundamental issue in the flow that prevents this?

@goodmind
Copy link
Contributor

I opened PR for this #7593 using library ocaml-dtoa that @mroch suggested

@Brianzchen
Copy link
Contributor

Brianzchen commented Apr 22, 2020

Still not supported as of v0.122.0. I'm just getting into flow and I'm enjoying the soundness but random issues like these that haven't been resolved or been given work around recommendations by flow maintainers makes it rather annoying.

Edit: If using numbers as keys are considered non-sound then it seems fine to leave the issue there and instead encourage string only values as keys which falls under a similar boat as another issues I previously had which I solved with converting it to an explicit string with string literals.

{
 [`${verticalPos === pos ? 'top' : 'bottom'}`]: ...
}

As for simple numbers, I opted for simply using them as strings

'123': 123,

This caused me an eslint error which I solved with this, which still keeps the rules enabled for regular cases.

'quote-props': ['error', 'as-needed', { keywords: false, unnecessary: true, numbers: true }],

chrisbobbe pushed a commit to gnprice/zulip-mobile that referenced this issue Jul 20, 2020
The way this file had worked for a while was that this flags object,
with these particular arbitrary message IDs, was used globally in
the file and then these two particular tests used message IDs that
just happened to have specific relationships to those here.

That's no good. :-)  It makes these tests harder to read, by obscuring
their relationship to the data they're actually testing; and it makes
all the other tests harder to read, by raising the possibility that
they might somehow depend on these oddly-specific `flags` values.

Instead, push this data down into the specific tests that use it.
Much clearer.

Also demonstrate how to avoid the Flow error: write the key as an
actual number.  The trick is just to tell ESLint to go away when it
tries to rewrite the key as a (numeric-looking) string. [1]

[1] facebook/flow#380

[chris: link to Flow issue in code and commit message]
saarCiklum pushed a commit to Folcon/js-xlsx that referenced this issue Aug 17, 2020
- README and example cleanup
- basic XLSB and ODS write support
- flow typecheck for ODS file
  Note: xlsx.js flow fails: facebook/flow#380
- exposed jszip compression (fixes SheetJS#220, closes SheetJS#284)

README issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| SheetJS#202 | @sao93859      | closes SheetJS#202                                  |
| SheetJS#211 | @alexanderchan | closes SheetJS#211 corrected examples               |
| SheetJS#327 | @cskaandorp    | changed saveAs example to match write tests  |
| SheetJS#424 | @dskrvk        | added note about s2roa h/t @LeonardoPatignio |
| SheetJS#496 | @jimmywarting  | closes SheetJS#496 adapted rABS examples with rAAS  |

ODS file format issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| protobi#148 | @user4815162342| closes protobi#148 h/t @ziacik                      |
| protobi#166 | @paulproteus   | closes protobi#166 rudimentary ODS write support    |
| protobi#177 | @ziacik        | closes protobi#177                                  |
| protobi#179 | @ziacik        | closes protobi#179 use JSON when available          |
| SheetJS#317 | @ziacik        | closes SheetJS#317                                  |
| SheetJS#328 | @think01       | closes SheetJS#328                                  |
| SheetJS#383 | @mdamt         | closes SheetJS#383 duplicate cells should be copied |
| SheetJS#430 | @RB-Lab        | closes SheetJS#430                                  |
| SheetJS#546 | @lgodard       | closes SheetJS#546 thanks to other changes          |
@dominikabieder
Copy link

I've upgraded prettier which prompted me to drop the quotes const X = { 2: "1" }, but flow is complaining. any advice?

@Brianzchen
Copy link
Contributor

I've upgraded prettier which prompted me to drop the quotes const X = { 2: "1" }, but flow is complaining. any advice?

Given that prettier doesn't have an option to configure this, probably the easiest solution to to write your obj as the following to retain the key as a string value.

const X = { ['2']: '1' }

I'd actually consider taking this up with the prettier team, since on their homepage they apparently support Flow

@nemoDreamer
Copy link

Making it a Map does seem like the more modern approach. Worked nicely for me because I had fixed integer keys and nothing else:

@@ -20,24 +25,24 @@ describe("utils", () => {
     describe("resolveDiceRoll", () => {
       const specs = {
         [PLAYER_0]: {
-          near: {
-            1: 4,
-            6: 9,
-          },
-          far: {
-            1: 0,
-            6: 5,
-          },
+          near: new Map([
+            [1, 4],
+            [6, 9],
+          ]),
+          far: new Map([
+            [1, 0],
+            [6, 5],
+          ]),
         },
         [PLAYER_1]: {

tbergquist-godaddy added a commit to adeira/universe that referenced this issue Nov 10, 2020
Turning this eslint rule off since flow doesn't work with non-string literal
property keys, facebook/flow#380.
tbergquist-godaddy added a commit to adeira/universe that referenced this issue Nov 10, 2020
Turning this eslint rule off since flow doesn't work with non-string literal
property keys, facebook/flow#380.
adeira-github-bot pushed a commit to adeira/eslint-config-adeira that referenced this issue Nov 10, 2020
Turning this eslint rule off since flow doesn't work with non-string literal
property keys, facebook/flow#380.

adeira-source-id: 96aef9065f11f6964f4789807183bb2dfa4e1c09
@jamesqo
Copy link

jamesqo commented Jan 3, 2021

It's kind of ridiculous that this issue is still present in 2021...

@ayroblu
Copy link

ayroblu commented May 27, 2021

Hey, I encountered this and tried it out:

> var a = { 1: 'hi'}
undefined
> a['1']
'hi'
> a[1]
'hi'
> var a = { 1: 'hi', '1': 'bye' }
undefined
> a
{ '1': 'bye' }

I think flow's decision here is correct? Perhaps inconvenient, but knowing the above I wouldn't use number types

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 8, 2022
Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'll make it difficult to construct valid test data for this part
of the /register response in a way that Flow can track, when we
enable `flow strict-local` in userStatusReducer-test.js soon.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 8, 2022
Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 11, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip@21c98f8#r802277662

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 16, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip@21c98f8#r802277662

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 19, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip@21c98f8#r802277662

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 23, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip#5224 (comment)

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this issue Feb 25, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip#5224 (comment)

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
gnprice pushed a commit to gnprice/zulip-mobile that referenced this issue Feb 25, 2022
All JS objects are string-keyed; see Greg's explanation at
  zulip#5224 (comment)

For convenience, Flow has some support for object indexer types that
aren't subtypes of `string`.

But Flow doesn't yet support numeric literal property keys; see
  facebook/flow#380
and
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZMUAIx1gFItG4BuKAHptUAHLCAtGIoBLAgHMozM8EptmUMBQaQKoKAGsIIEVAIGcREiMDAGDwgAEwA6fCJRUPDIqJMQAmA2AA8qIA

That'd make it hard to construct valid test data for this part of
the /register response in a way that Flow can track, when we start
type-checking userStatusReducer's test file soon. At least, it'd
make it hard to use literal values to construct test data.

It'd be tempting to use a workaround mentioned in that Flow issue,
  facebook/flow#380 (comment) :

```
var x = {
    [1234]:"a",
    [5678]:"b"
}
```

but then it seems we'd silently lose type-checking of the values at
numeric keys in the object; see
  https://flow.org/try/#0C4TwDgpgBAsiDyAjAVlAvFA3gHygbQDsBXAW0QgCcBdALikQHsGAbCAQwKmwF8AoXgMYMCAZ2BQ2dOElQZM+AIy0oAcgBmTFVG4BuKAHp9UAMoALBkWYATKJQoMKUABTrNUAJYiCK8W3pNWDgBCAEogA :

```
type MyObj = {| [number]: boolean |}

// Should error ('foo' isn't a boolean!)
const a: MyObj = { [1]: 'foo' };
```

Fortunately, I don't think there's a downside to saying the
properties are strings here. This type no longer has anything to do
with the type we use for our internal "user status" model. It's just
a read-only data object whose keys we'll read one by one, to inform
how the model updates on a REGISTER_COMPLETE. All of JavaScript's
built-in methods for reading an object's keys (e.g., Object.keys)
give the keys as strings, that we'll have to call
makeUserId(parseInt(…)) on.
KingTiger001 added a commit to KingTiger001/sheet-project that referenced this issue Dec 18, 2022
- README and example cleanup
- basic XLSB and ODS write support
- flow typecheck for ODS file
  Note: xlsx.js flow fails: facebook/flow#380
- exposed jszip compression (fixes #220, closes #284)

README issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| #202 | @sao93859      | closes #202                                  |
| #211 | @alexanderchan | closes #211 corrected examples               |
| #327 | @cskaandorp    | changed saveAs example to match write tests  |
| #424 | @dskrvk        | added note about s2roa h/t @LeonardoPatignio |
| #496 | @jimmywarting  | closes #496 adapted rABS examples with rAAS  |

ODS file format issues:

|  id  | author         | comment                                      |
|-----:|:---------------|:---------------------------------------------|
| #148 | @user4815162342| closes #148 h/t @ziacik                      |
| #166 | @paulproteus   | closes #166 rudimentary ODS write support    |
| #177 | @ziacik        | closes #177                                  |
| #179 | @ziacik        | closes #179 use JSON when available          |
| #317 | @ziacik        | closes #317                                  |
| #328 | @think01       | closes #328                                  |
| #383 | @mdamt         | closes #383 duplicate cells should be copied |
| #430 | @RB-Lab        | closes #430                                  |
| #546 | @lgodard       | closes #546 thanks to other changes          |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Has PR incompleteness Something is missing
Projects
None yet
Development

Successfully merging a pull request may close this issue.