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

Support watching lists in debugger #27628

Merged
merged 4 commits into from Mar 22, 2019
Merged

Support watching lists in debugger #27628

merged 4 commits into from Mar 22, 2019

Conversation

jmkulwik
Copy link
Contributor

No description provided.

@jmkulwik jmkulwik requested review from epeach and ajpal March 21, 2019 22:14
@jmkulwik
Copy link
Contributor Author

Before:
image

After:
image

@jmkulwik jmkulwik changed the title Debugger arrays Support watching lists in debugger Mar 21, 2019
Copy link
Contributor

@ajpal ajpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we don't want long string values to wrap also?

let parsedArray = '';
array.forEach((element, index, array) => {
if (element === null) {
parsedArray = parsedArray + 'null';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: += would be more concise

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Was still thinking in python from teaching in the AM. :)

if (element === null) {
parsedArray = parsedArray + 'null';
} else if (Array.isArray(element)) {
parsedArray = parsedArray + 'list (' + element.length + ')';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should/could this recursively parse so that watching a list of lists would show you the contents of all the lists?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also interested in this^?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to for a couple reasons:

  1. It starts to look confusing when you have multiple lists or multiple levels of nesting in a list. The way our UI currently is set up, you end up with a bunch of commas, and it's difficult to figure out which array each element belongs to.

  2. I checked how the chrome debugger handles this case, and it does something similar. It doesn't expand nested arrays by default. (the exception to this is it will if there is only a single level of nesting)
    Chrome example:
    image

@jmkulwik
Copy link
Contributor Author

Is there a reason we don't want long string values to wrap also?

It would be nice, but with the way the spacing is currently set up, there isn't a graceful way for it to be vertically centered both when the string is short and when it wraps. The array formatting cheats a little because it will always be a multi-line display.

I'm opening a ticket for this in the backlog, however. It also might be worth revisiting how this watch window is set up.

@jmkulwik jmkulwik merged commit 11440c1 into staging Mar 22, 2019
@jmkulwik jmkulwik deleted the debugger-arrays branch May 20, 2019 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants