1371936 - Objects with only numeric keys are represented as an Array

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P2)

Description

[Sukant Garg [:gargsms]]

The console incorrectly displays Objects with only numeric keys as Arrays

Typing in the console:
    console.log( { 1: "one", 4: "four", 0: "zero" } )

produces this:
    Object [ "zero", "one", <2 empty slots>, "four" ]

This has existed since Firefox 51, as I verified the correct output in FF50 and earlier.

Possibly related to: https://bugzilla.mozilla.org/show_bug.cgi?id=1295729

Comment 1

[Brian Grinstead [:bgrins]]

Hi Nicolas, do we have a reps bug on file for this?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

No we don't, but I think this is an intended behavior [1] (and we have it on both old and new console).
Now maybe this should be true only if keys are truly subsequent ?


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1096294

Comment 3

[Sukant Garg [:gargsms]]

I am not sure if it was available in old consoles. It was certainly not reproducible in FF50.
And no, it does not matter if the keys are subsequent.

console.log({1: 'one', 99: 'ninety nine'}) // Object [ <1 empty slot>, "one", <8 empty slots>, 90 more… ]

Maybe a check for subsequent keys would help in this case.

As for me, I did not expect this behaviour, and only came to realize that it is not a bug when I came across [1]



[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1096294

Comment 4

[shimscharf]

This should not be expected behaviour of an object -> this should be the expected behaviour of an array

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Patrick, this behavior comes from Bug 1096294, and I wonder if this is still needed.
I have mixed feelings about this. The original idea to save some screen estate is nice, bit it also can mislead the user to think the logged object is indeed an array (even if we display `Object ["one"]` for now, with object inspection we might remove the title to save space `▶︎ ["one"]`).

Also, I think this can be plainly bad for Object used as a map for example ({`[id]: itemData}`).
In this case, if you have one id less than 10, it will show as an array, if not, it will show as an object.

- `{1: 1}` -> Object [ <1 empty slot>, 1 ]
- `{1909: 1}` -> Object { 1909: 1 }

A simple fix regarding the test case from comment 0 would be to say that the object is ArrayLike if and only if it has subsequent indexed keys, and even like that I'm not convinced about that.


Also, we plan to experiment with custom formatters for the console in Q3, so I guess some libraries/app that uses this kind of indexed objects could have better output provided by the library/app owner.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #5)
> Patrick, this behavior comes from Bug 1096294, and I wonder if this is still
> needed.
It was added for a specific use case, where jquery (maybe other frameworks too) was using an object to list nodes, instead of an array. We had received user feedback that this would be a good thing to add. Jquery was returning an object with numeric indexes only, from 0 to N, without gaps.
I think it made sense to display those like arrays, because it made it easier to check the output.

> I have mixed feelings about this. The original idea to save some screen
> estate is nice, bit it also can mislead the user to think the logged object
> is indeed an array (even if we display `Object ["one"]` for now, with object
> inspection we might remove the title to save space `▶︎ ["one"]`).
> 
> Also, I think this can be plainly bad for Object used as a map for example
> ({`[id]: itemData}`).
> In this case, if you have one id less than 10, it will show as an array, if
> not, it will show as an object.
> 
> - `{1: 1}` -> Object [ <1 empty slot>, 1 ]
> - `{1909: 1}` -> Object { 1909: 1 }
> 
> A simple fix regarding the test case from comment 0 would be to say that the
> object is ArrayLike if and only if it has subsequent indexed keys, and even
> like that I'm not convinced about that.
Yeah I don't understand why we implemented it in this way. I thought the goal was to only do this for cases where you do have only numeric and subsequent keys starting at 0 only. Also, I don't understand why we stop at 10?
I agree that in the current implementation it's confusing.

> Also, we plan to experiment with custom formatters for the console in Q3, so
> I guess some libraries/app that uses this kind of indexed objects could have
> better output provided by the library/app owner.
This sounds like a much better solution to the use case indeed!

Comment 7

[cr7-rm]

In my opinion this should be fixed soon as this behaviour mixes the view of two completly different data types in JavaScript. An object should always be displayed as a group of key-value pairs. It would be a great thing to enable this feature in the console settings ('Display array like objects as arrays'). But then the object should have numeric keys only and a length property. The latter is currently not needed.

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Bumping the priority since we got multiple bug reports on this lately.

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

We only label Object as being ArrayLike if they have consecutive, numeric indexes, starting at 0,
and that could contain only a non-numeric length property that matches the actual number of numeric
keys in the object.
A test is added to make sure we don't regress this.

Review commit: https://reviewboard.mozilla.org/r/200338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200338/

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

https://reviewboard.mozilla.org/r/200338/#review205448

Seems sensible to prevent more things from falling into this preview type.  Thanks for adding the test

::: devtools/server/actors/object.js:1946
(Diff revision 1)
>        return false;
>      }
>  
> -    // If no item is going to be displayed in preview, better display as sparse object.
> -    // The first key should contain the smallest integer index (if any).
> -    if (keys[0] >= OBJECT_PREVIEW_MAX_ITEMS) {
> +    // We don't want to represent Objects as sparse arrays, so every property
> +    // should match its index, or be the length property.
> +    if (keys.some((key, i) => parseInt(key, 10) !== i && key !== "length")) {

Should we keep the `keys[0] >= OBJECT_PREVIEW_MAX_ITEMS` condition in front of this one to prevent iterating the entire object in this case?

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

https://reviewboard.mozilla.org/r/200338/#review205448

> Should we keep the `keys[0] >= OBJECT_PREVIEW_MAX_ITEMS` condition in front of this one to prevent iterating the entire object in this case?

`Array.some` will stop iterating as soon as a truthy value is returned, e.g.
```
[13,14,15].some((key, i) => console.log(key) || parseInt(key, 10) !== i)
```
will only print `13` and stop.

So I think it's safe to not keep the old test (and we would have to change it to `parseInt(keys[0], 10) === 0` , which is redundant with what the `some` callback is doing).

Comment 15

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #14)
> So I think it's safe to not keep the old test (and we would have to change
> it to `parseInt(keys[0], 10) === 0` , which is redundant with what the
> `some` callback is doing).

OK, makes sense

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200338/diff/1-2/

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

Asking for review again since some old console frontend tests changed

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

https://reviewboard.mozilla.org/r/200338/#review206032

Ah, thanks for checking on that before landing

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57596d25bebe
Fix erroneous Object as ArrayLike grip; r=bgrins.

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/57596d25bebe

Comment 21

[Oriol Brufau [:Oriol]]

Hi, my idea was something like this:

 - `{1: 1, length: 2}`             -> Object [ <1 empty slot>, 1 ]
 - `{1: 1, 1909: 1, length: 1910}` -> Object [ 1, <1907 empty slots>, 1 ]
 - `{1909: 1, length: 1910}`       -> Object [ <1909 empty slots>, 1909: 1 }

But array previews are not sparse, so it ended up being suboptimal.

So maybe restricting to non-sparse array-like makes more sense, yes.

But note parseInt is not the proper way to detect array indices.
Not a big problem because the proper check is done later in the existing loop,
but this is redundant.
In fact, assuming getOwnPropertyNames will order them like OrdinaryOwnPropertyKeys
(and this assumption has already been made somewhere else), then no loop is needed.

Comment 23

[buhhcz]

Isn't that bug deserve a hotfix for stable version? Especially now when Firefox "relaunched".

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

A hotfix to release I am not sure, but maybe we can ask the uplift to beta since the changes are pretty small and are well tested.

Comment 28

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 55.0a1 (2017-06-10)on Windows 10, 64 Bit!

This bug's fix is verified with latest Nightly!

Build ID   : 20180112100121
User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0