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.

Sign up for GitHub

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

Rename from iterator to iterable and split the functionality #19831

Merged
merged 9 commits into from
Sep 22, 2020
Merged

Rename from iterator to iterable and split the functionality #19831

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ca2d02d
rename from iterator to iterable and split the functionality
todortotev Sep 14, 2020
60b8bb5
brought back the iterator variable name due to tests failing
todortotev Sep 18, 2020
2cf0346
fix the issue with failing CI
todortotev Sep 18, 2020
ce81af1
remove parentheses before data
todortotev Sep 18, 2020
2558946
move the check in the iterator case
todortotev Sep 18, 2020
c629822
check if executing the function returns an object
todortotev Sep 18, 2020
cf19623
remove symbol import
todortotev Sep 22, 2020
f7052e8
modify the check once again
todortotev Sep 22, 2020
28bb959
Added tests and fixed edge cases for DevTools iterator handling
Sep 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions packages/react-devtools-shared/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,7 @@ export type DataType =
| 'html_element'
| 'infinity'
| 'iterator'
| 'opaque_iterator'
| 'nan'
| 'null'
| 'number'
Expand Down Expand Up @@ -437,6 +438,8 @@ export function getDataType(data: Object): DataType {
return 'array_buffer';
} else if (typeof data[Symbol.iterator] === 'function') {
return 'iterator';
} else if (data[Symbol.iterator] === 'data') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. Why would data[Symbol.iterator] return the string "data"?

return 'opaque_iterator';
} else if (data.constructor && data.constructor.name === 'RegExp') {
return 'regexp';
} else {
Expand Down Expand Up @@ -615,11 +618,6 @@ export function formatDataForPreview(
}
case 'iterator':
const name = data.constructor.name;
// We check if the the generator returns itself.
// If it does, we want to avoid to iterate over it
if (typeof data[Symbol.iterator]() === 'object') {
return `${name}(${data.size})`;
}

if (showFormattedValue) {
// TRICKY
Expand Down Expand Up @@ -659,6 +657,9 @@ export function formatDataForPreview(
} else {
return `${name}(${data.size})`;
}
case 'opaque_iterator': {
return `${data.constructor.name}(${data.size})`;
Copy link
Contributor

@bvaughn bvaughn Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think this will result in the string "(undefined)" (at least based on my testing, using a similar approach as mentioned in #19726).

data.constructor points at the generator function, which has no .name property. Maybe we should use data[Symbol.toStringTag] instead? Although that's just "Generator" in my testing, so maybe it's not really that useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just String(data) can work for all

Copy link
Contributor

@bvaughn bvaughn Sep 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That would result in a different format than we're looking for here ("[object Generator]")

Copy link
Contributor

Choose a reason for hiding this comment

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

${data.constructor.name}(${data.size}) I think we cannot have this format, because we cannot have the size of generator

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know. That's what I commented above :)

}
case 'date':
return data.toString();
case 'object':
Expand Down