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

Add algorithm for console table #237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GasimGasimzada
Copy link

@GasimGasimzada GasimGasimzada commented Aug 11, 2024

Define algorithm for console.table

  • Define the data structure that is passed to the Formatter
  • Create algorithm that creates the final structure based on tabularData and properties arguments

Description

I have tried to figure out and define the algorithm for console.table based on how Chromium and Firefox do it. This is my first PR; so, please let me know of any concerns and comments.

Checklist

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

@zcorpan
Copy link
Member

zcorpan commented Aug 15, 2024

Does the algorithm match Chrome or Firefox, or neither? cc @nchevobbe

@GasimGasimzada GasimGasimzada force-pushed the console-table-algorithm branch 3 times, most recently from 1f2faf9 to dc65c23 Compare August 15, 2024 16:11
@GasimGasimzada
Copy link
Author

Does the algorithm match Chrome or Firefox, or neither? cc @nchevobbe

@zcorpan It matches both Chromium and Firefox. I did not see any difference between them for the cases that I tested.

@nchevobbe
Copy link

nchevobbe commented Aug 15, 2024

Does the algorithm match Chrome or Firefox, or neither? cc @nchevobbe

@zcorpan It matches both Chromium and Firefox. I did not see any difference between them for the cases that I tested.

There's one difference I'm aware of (see https://bugzilla.mozilla.org/show_bug.cgi?id=1744441)

With

var client=[];
client["a"]=[1];
client["b"]=[2];
console.table(client);

Firefox and Safari don't print a table, Chrome and Node do.
Given

3. If |tabularData| is a [=/list=], then:
  1. Let |indices| be [=list/get the indices=] of |tabularData|

and https://infra.spec.whatwg.org/#lists :

To get the indices of a list, return the range from 0 to the list’s size, exclusive.

https://infra.spec.whatwg.org/#the-exclusive-range

The range n to m, exclusive, creates a new ordered set containing all of the integers from n up to and including m − 1 in consecutively increasing order, as long as m is greater than n. If m equals n, then it creates an empty ordered set.

I guess non integer indices shouldn't be included, which is what Safari and Firefox are doing

@GasimGasimzada
Copy link
Author

I guess non integer indices shouldn't be included, which is what Safari and Firefox are doing

I did not check the use case of setting non integer indices to an array. Personally, my preference is that arrays only handle integer indices which is aligned with what Safari and Firefox (and LadyBird once my PR is ready to be merged :D) are doing. But it is just a personal preference and I do not have a strong opinion either way. Let me know how you want to proceed with this.

@AtkinsSJ
Copy link
Contributor

This is now live in Ladybird (LadybirdBrowser/ladybird#1027) if anyone wants to experiment with it.

@zcorpan
Copy link
Member

zcorpan commented Aug 22, 2024

@bmeurer can this be changed in Chromium/V8? See #237 (comment) above.

@bmeurer
Copy link

bmeurer commented Aug 22, 2024

can this be changed in Chromium/V8? See #237 (comment) above.

We can definitely update Chromium accordingly, but I wonder if that wouldn't be considered a regression. Being able to print objects (with non-indexed properties) as tables could be useful for some users. Is there some data that we can base the decision to stick to just indexed properties on?

@zcorpan
Copy link
Member

zcorpan commented Aug 22, 2024

I think the idea is that a JS object (that is not an array) would still work (step 4).

Hmm, the spec algorithm here operates on Infra value types (list vs map), but the IDL type for JS arrays and JS objects are both "object" if I understand https://webidl.spec.whatwg.org/#js-any correctly. ( https://webidl.spec.whatwg.org/#js-union can map to an IDL sequence, step 11.1.)

I'm not sure what the JS - IDL - Infra mapping is for an IDL any type... If a JS array maps to IDL object and then Infra map, then https://github.com/whatwg/console/pull/237/files#diff-5e793325cd2bfc452e268a4aa2f02b4024dd9584bd1db3c2595f61f1ecf7b985R129 is always false.

@GasimGasimzada
Copy link
Author

I'm not sure what the JS - IDL - Infra mapping is for an IDL any type... If a JS array maps to IDL object and then Infra map, then https://github.com/whatwg/console/pull/237/files#diff-5e793325cd2bfc452e268a4aa2f02b4024dd9584bd1db3c2595f61f1ecf7b985R129 is always false.

@zcorpan I am new to writing a spec and this is something I was not aware of. Just want to understand this for this use-case and as a future reference as well:

  1. Since JS array is a JS object underneath, should I treat both array and object as a map?
  2. If (1) is true, if I want to only iterate over indexed values of a JS object, how would I express this? I am asking this because there is no method in map for getting indices of an item while there is a method on it in the list.

@domenic
Copy link
Member

domenic commented Aug 23, 2024

The spec written here is not clear enough one way or another.

tabularData is specified as any. any is a Web IDL type representing an opaque handle to a JavaScript value, so it can never be an Infra list or map. So right now the spec never enters steps 3, 4, or 5 and always falls back to step 6.

You need to either:

  • Express a more clear type than any for tabularData. E.g. maybe (sequence<any> or record<DOMString, any>). Note that this can throw during conversion from JS to Web IDL, e.g. if [Symbol.iterator] is a throwing getter, or if there are in general any throwing getters as keys.
  • Use raw ECMAScript spec operations to manipulate tabularData. This allows you full control over the handling of exceptional cases.

The former would be better but I'm unsure how compatible it is with current implementations.

@bmeurer
Copy link

bmeurer commented Aug 23, 2024

I think the idea is that a JS object (that is not an array) would still work (step 4).

Ah, I misread the example. If it's only about excluding non-indexed properties for arrays (or array-like objects), we can definitely do that in Chromium.

@zcorpan
Copy link
Member

zcorpan commented Aug 23, 2024

@domenic I think the second option is closer to what browsers do today. Gecko uses any. This doesn't throw in Safari/Chrome/Firefox:

let o = {}; Object.defineProperty(o, "x", { get() { throw "y" } }); console.table(o);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants