Closed Bug 1371936 Opened 3 years ago Closed 2 years ago

Objects with only numeric keys are represented as an Array

Categories

(DevTools :: Console, enhancement, P2)

x86_64
Linux
enhancement

Tracking

(firefox59 fixed)

RESOLVED FIXED
Firefox 59
Tracking Status
firefox59 --- fixed

People

(Reporter: gargsms, Assigned: nchevobbe)

References

Details

Attachments

(1 file)

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
Hi Nicolas, do we have a reps bug on file for this?
Flags: needinfo?(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
Flags: needinfo?(nchevobbe)
Flags: needinfo?(garg_sms)
Flags: needinfo?(bgrinstead)
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
Flags: needinfo?(garg_sms)
This should not be expected behaviour of an object -> this should be the expected behaviour of an array
Priority: -- → P3
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.
Flags: needinfo?(bgrinstead) → needinfo?(pbrosset)
(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!
Flags: needinfo?(pbrosset)
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.
Duplicate of this bug: 1413717
Duplicate of this bug: 1417406
Duplicate of this bug: 1417486
Bumping the priority since we got multiple bug reports on this lately.
Priority: P3 → P2
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
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?
Attachment #8929025 - Flags: review?(bgrinstead) → review+
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).
(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 on attachment 8929025 [details]
Bug 1371936 - Fix erroneous Object as ArrayLike grip; .

Asking for review again since some old console frontend tests changed
Attachment #8929025 - Flags: review+ → review?(bgrinstead)
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
Attachment #8929025 - Flags: review?(bgrinstead) → review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/57596d25bebe
Fix erroneous Object as ArrayLike grip; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/57596d25bebe
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
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.
Depends on: 1419447
Duplicate of this bug: 1419730
Isn't that bug deserve a hotfix for stable version? Especially now when Firefox "relaunched".
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.
Duplicate of this bug: 1420501
Duplicate of this bug: 1421574
Duplicate of this bug: 1427758
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
QA Whiteboard: [bugday-20180110]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.