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

Use correct object context for expanding accessors in the prototype chain #108036

Closed
Spown opened this issue Oct 3, 2020 · 15 comments
Closed

Use correct object context for expanding accessors in the prototype chain #108036

Spown opened this issue Oct 3, 2020 · 15 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues polish Cleanup and polish issue verified Verification succeeded
Milestone

Comments

@Spown
Copy link

Spown commented Oct 3, 2020

Since the couple last versions, when debugging JS and inspecting the variables (both in side menu and by hovering over code) the setter/getter methods are prefixed with "set/get". I get why it is useful, however the implementation messes with one's workflow. Personally I expect variable/property list to be alphabetical, so I focus on the first letters first, which speeds up it's traversal. But now it's a mess.

Can you add an icon or color code them or move "get/set" word behind the name, like:
set foo: ⨍ () -> foo: set ⨍ ()
or something of this sort, instead of using text?

Or is there a config option already for controlling this?

Thank you.

@connor4312
Copy link
Member

You can use the setting debug.javascript.autoExpandGetters to automatically expand those.

@Spown
Copy link
Author

Spown commented Oct 3, 2020

@connor4312 I've tried it. it changes nothing.

@connor4312
Copy link
Member

Make sure you're using type: pwa-node or type: pwa-chrome, rather than their non-prefixed versions

@Spown
Copy link
Author

Spown commented Oct 3, 2020

@connor4312 I can see no difference with pwa-node and debug.javascript.autoExpandGetters. please reopen.

image

@connor4312
Copy link
Member

Can you please grab some logs with that setting applied and using pwa-node?

/jsDebugLogs

@Spown
Copy link
Author

Spown commented Oct 4, 2020

sent to connor@xbox.com. stripped some sensitive data.

@connor4312
Copy link
Member

Thanks for the logs. Looks like we are trying to expand the getters, but accessing them fails, e.g. trying to call return this['participants'] on the prototype results in an error:

TypeError: Cannot read property 'Symbol(mongoose#Document#scope)' of undefined
    at model.get [as created] (F:\\Projects\\█████\\█████\\node_modules\\mongoose\\lib\\helpers\\document\\compile.js:158:45)
    at model.<anonymous> (eval-fde7c78c.cdp:1:25)
    at Object.module.exports (F:\\Projects\\█████\\█████\\pages\\admin__history.js:23:13)
    at F:\\Projects\\█████\\█████\\node_modules\\ecms\\platform.js:1049:71
    at Promise.post (F:\\Projects\\█████\\█████\\node_modules\\q\\q.js:1179:30)
    at Promise.promise.promiseDispatch (F:\\Projects\\█████\\█████\\node_modules\\q\\q.js:808:41)
    at F:\\Projects\\█████\\█████\\node_modules\\q\\q.js:1411:14
    at runSingle (F:\\Projects\\█████\\█████\\node_modules\\q\\q.js:137:13)
    at flush (F:\\Projects\\█████\\█████\\node_modules\\q\\q.js:125:13)
    at process._tickCallback (internal/process/next_tick.js:61:11)

I (assume) the reason is that we set this to be the prototype object. We should introduce some special handling so that retrieving accessors in the prototype chain use the object itself rather than the prototype.

@connor4312 connor4312 changed the title Please remove "set/get" prefixes from variable inspector Use correct object context for expanding accessors in the prototype chain Oct 4, 2020
@connor4312 connor4312 added bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues and removed info-needed Issue requires more information from poster labels Oct 4, 2020
@connor4312 connor4312 added this to the October 2020 milestone Oct 4, 2020
@Spown
Copy link
Author

Spown commented Oct 5, 2020

Thank you for the swift reaction. However I wonder, since the issue was reclassified into a bug-report...

  1. would the fix actually change how the property/method/var name is depicted (i.e. will the get/set disappear from the name string) with debug.javascript.autoExpandGetters=true? Because the option discription only mentions getters. What about the setters?
  2. I'm not sure I want the value being evaluated every time I expand it, since mongoose sets some internal hooks there. The inspection better be as zen as possible. That is if I understand correctly what this option does.

@connor4312
Copy link
Member

connor4312 commented Oct 5, 2020

would the fix actually change how the property/method/var name is depicted (i.e. will the get/set disappear from the name string) with debug.javascript.autoExpandGetters=true? Because the option discription only mentions getters. What about the setters?

The setter still shows up, e.g.:

I'm not sure I want the value being evaluated every time I expand it, since mongoose sets some internal hooks there. The inspection better be as zen as possible. That is if I understand correctly what this option does.

That is why this option was introduced 🙂 The previous debugger always evaluated+expanded getters on objects. Chrome's debugger doesn't do so for the reason you mentioned -- though generally considered bad practice, getters can have side-effects -- and I replicated that behavior in the new debugger for the same reason, with an option to use the old behavio.

@Spown
Copy link
Author

Spown commented Oct 5, 2020

The setter still shows up

So, is there something I could do to hide it? Or change the way how it appears in the tree? Something like set foo: ⨍ () -> foo: setter ⨍ () would really be nice...

Can you perhaps do something about it, since you are already there? Or should I make a separate issue?

Thank you.

@connor4312
Copy link
Member

@Spown currently these are not easily customizable. I prefer to keep the set foo over foo: setter since it's more clear how it's defined (the variables view actually shows the same syntax as the user is using in this code to define the setter/getter)


Trying to repro this, I'm not sure there's actually a bug -- using the getting started on Mongoose's docs, I can see the same un-expanded properties that you're seeing, but trying to access them in the repl gives the same error

Uncaught TypeError: Cannot read property 'Symbol(mongoose#Document#scope)' of undefined

So far as far I can see we're working as designed.

@connor4312 connor4312 added the info-needed Issue requires more information from poster label Oct 5, 2020
@Spown
Copy link
Author

Spown commented Oct 5, 2020

I prefer to keep the set foo over foo: setter since it's more clear how it's defined

Well, for me finding the right property/var first is much more important than to know how it came to be. If I need to know how it is defined - I can look into it specifically. But traversal is something that I always need. And having the list in alphabetical order is a must. Now it is only ordered alphabetically, but represented with words starting with "s" and "g" mixed all over the place. It wasn't like that before and now suddenly its a new norm without an option to opt out.

@connor4312 connor4312 removed the info-needed Issue requires more information from poster label Oct 13, 2020
@connor4312
Copy link
Member

connor4312 commented Oct 13, 2020

Fixed in the linked commit. Getters/settings now appear as foo (get): rather than foo (set):. This is different than Chrome, but closer to the nice experience that Firefox devtools provide: (firefox screenshot)

We don't have the capability, as a debugger, to display quite as pretty a display, but I agree that preserving lexographic order is preferable.

Available in the next nightly

@connor4312 connor4312 added bug Issue identified by VS Code Team member as probable bug polish Cleanup and polish issue and removed bug Issue identified by VS Code Team member as probable bug labels Oct 13, 2020
@aeschli aeschli added the verified Verification succeeded label Oct 29, 2020
@aeschli
Copy link
Contributor

aeschli commented Oct 29, 2020

Verified that getters/setters are now rendered with x (get) resp. x (set)

@github-actions github-actions bot locked and limited conversation to collaborators Dec 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debug Debug viewlet, configurations, breakpoints, adapter issues polish Cleanup and polish issue verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants
@isidorn @connor4312 @Spown @aeschli and others