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

Show Set and Map expanded views #490

Closed
wants to merge 6 commits into from

Conversation

AdamNiederer
Copy link

@AdamNiederer AdamNiederer commented Jan 6, 2018

Adds a drop-down for sets and maps, and also shows the number of entries in Objects to maintain uniformity.

Partially _resolves #354 and resolves #349.

We have access to it, so we should show it.
Y'all made me think there was a bug in my stringifier and regenerator
Copy link
Member

@Akryum Akryum left a comment

Choose a reason for hiding this comment

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

I have put some pieces of advice. Also, we may wait for #486 that will allow us to do things like this.

h: 'I am a really long string mostly just to see how the horizontal scrolling works.'
h: 'I am a really long string mostly just to see how the horizontal scrolling works.',
i: new Set([1, 2, 3, 4, new Set([5, 6, 7, 8])]),
j: new Map([[1, 2], [3, 4], [5, new Map([[6, 7,]])]])
Copy link
Member

Choose a reason for hiding this comment

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

Did you try to put a Set inside a Map and the other way around?

Copy link
Author

Choose a reason for hiding this comment

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

Yes; I've added a more complex test case in db270b3.

@@ -81,7 +81,7 @@ function initApp (shell) {
})

bridge.on('instance-details', details => {
store.commit('components/RECEIVE_INSTANCE_DETAILS', parse(details))
store.commit('components/RECEIVE_INSTANCE_DETAILS', parse(details, true))
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't revive data on devtools side.

Copy link
Author

Choose a reason for hiding this comment

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

Objects, Arrays, Numbers, Strings, and null are already revived on master. Is there any reason we don't want to do this for Sets, Maps, INF, undefined, and NaN?

Copy link
Member

Choose a reason for hiding this comment

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

We only revive JSON-compatible data (which means no 'reviving', just like a standard JSON.parse()) because we may need to send it back again (state edition and Vuex time traveling).

Copy link
Member

@Akryum Akryum Jan 6, 2018

Choose a reason for hiding this comment

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

Also, the upcoming State edition system expects JSON-compatible data to work (even if the user doesn't send any change).

Copy link
Author

Choose a reason for hiding this comment

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

Ah, that makes sense. I've moved the set reconstruction logic into the component, so the store should only contain JSON-compatible data now.

},
formattedValue () {
const value = this.field.value
if (value === null) {
return 'null'
} else if (value === UNDEFINED) {
} else if (value === UNDEFINED || value === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

(See below, data shouldn't be revived on devtools side.)

return 'undefined'
} else if (value === NAN) {
} else if (value === NAN || Number.isNaN(value)) {
Copy link
Member

Choose a reason for hiding this comment

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

(Same)

return 'NaN'
} else if (value === INFINITY) {
} else if (value === INFINITY || value === Number.POSITIVE_INFINITY) {
Copy link
Member

Choose a reason for hiding this comment

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

(Same)

@Akryum Akryum self-assigned this Jan 14, 2018
Akryum pushed a commit to Akryum/vue-devtools that referenced this pull request Jan 19, 2018
@Akryum
Copy link
Member

Akryum commented Jan 19, 2018

For now, we will continue with #523
Thank you very much for your involvement! 👍

@Akryum Akryum closed this Jan 19, 2018
Akryum pushed a commit that referenced this pull request Jan 22, 2018
* More fault tolerant

* Display stores with CustomValue API

* Dsiplay VueRouter instances with CustomValue API

* CustomValue API: new 'tooltip' option, supports functions, component def

* Test router & store

* Open in editor Component defs

* Fix #528

* Comments

* Encode cache clear

* Large array test

* CustomValue API: respect fields order

* Switch props and data sections to follow style guide

* New 'grey' and 'red' classes

DataField: allowing to reproduce current available styling using CustomValue API

* Add support for Map and Set - Fix #349

Initial PR #490 by AdamNiederer

* Set and Map test case

* Fix Map key/value inverted

* Optimizations

* Fix tooltip

* DataField style changes

* Env: added isLinux & isWindows + function 'f' style fixes

* gps-not-fixed icon is ugly on Windows

* Style tweaks

* Typo

* Added more function test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants