-
Notifications
You must be signed in to change notification settings - Fork 83
support basic scope in evaluate calls #344
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
Conversation
@@ -246,51 +246,89 @@ require("dart_sdk").developer.invokeExtension( | |||
@override | |||
Future evaluate(String isolateId, String targetId, String expression, | |||
{Map<String, String> scope, bool disableBreakpoints}) async { | |||
scope ??= {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why not use named parameters instead of ??=
With future non-nullability named parameters will be just as good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we have non-nullability, sure. Until then named parameter defaults should never be used imo.
var arguments = scope.values.map((id) => {'objectId': id}).toList(); | ||
var evalExpression = ''' | ||
function($argsString) { | ||
${_getLibrarySnippet(library.uri)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a TODO that we should cache the results of calling _getLibrarySnippet
and inject the library instance captured as another argument to the function. At that point it would make sense to unify the if and else cases here as they will have the same struture.
This gets rid of some exceptions in devtools when selecting widgets.
Next up is implementing
getObject
for dart instances so that hovering over things works.