Skip to content

Remove eval error message on async frames #2073

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

Merged

Conversation

annagrin
Copy link
Contributor

@annagrin annagrin commented Apr 4, 2023

Closes: #2057

Anna Gringauze added 2 commits April 4, 2023 16:33
@annagrin annagrin requested a review from elliette April 5, 2023 17:47
@@ -13,16 +13,23 @@ import 'package:dwds/src/utilities/objects.dart' as chrome;
import 'package:logging/logging.dart';
import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';

class ErrorKind {
const ErrorKind._(this._kind);
class EvaluationErrorKind {
Copy link
Contributor

@elliette elliette Apr 5, 2023

Choose a reason for hiding this comment

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

I think this would make sense as an enhanced enum, with methods such as toDisplayName / fromDisplayName so that we don't have to do the String matching (eg type.startsWith('${EvaluationErrorKind.compilation}'))

Copy link
Contributor Author

@annagrin annagrin Apr 5, 2023

Choose a reason for hiding this comment

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

I believe it is not really possible to express what I wanted here in dart, which would be something like:

enum EvaluationErrorKind {
  compilation = 'CompilationError',
  reference = 'ReferenceError',
 ...
}

So i'll have to resort to the usual "define a bunch of const strings and add comments on function parameters" that we use everywhere:) I'll do that to match other use cases, but I don't like losing the type safety of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@elliette
Copy link
Contributor

elliette commented Apr 5, 2023

I wonder if we could test this by providing a fake debugger (like we're providing a "real" debugger here)

final debugger = await Debugger.create(
webkitDebugger,
(_, __) {},
locations,
skipLists,
root,
);
which returns null for jsFrameForIndex.

I think ideally we need to add better support for unit testing and mocks though: #1643

@annagrin
Copy link
Contributor Author

annagrin commented Apr 6, 2023

I wonder if we could test this by providing a fake debugger (like we're providing a "real" debugger here)

final debugger = await Debugger.create(
webkitDebugger,
(_, __) {},
locations,
skipLists,
root,
);

which returns null for jsFrameForIndex.
I think ideally we need to add better support for unit testing and mocks though: #1643

Many other things will not work in the fake debugger (like location mapping etc) so I think we can do it as part of work for adding more unit tests and mocks. I'll think about it in the background as well, but for now I've found a way to test a real scenario - I figured out the way to hit the async code consistently, and it is matching flutter tools scenario (the code I keep hitting there when pausing is part of registering extensions machinery).

@annagrin
Copy link
Contributor Author

annagrin commented Apr 6, 2023

I wonder if we could test this by providing a fake debugger (like we're providing a "real" debugger here)

final debugger = await Debugger.create(
webkitDebugger,
(_, __) {},
locations,
skipLists,
root,
);

which returns null for jsFrameForIndex.
I think ideally we need to add better support for unit testing and mocks though: #1643

Many other things will not work in the fake debugger (like location mapping etc) so I think we can do it as part of work for adding more unit tests and mocks. I'll think about it in the background as well, but for now I've found a way to test a real scenario - I figured out the way to hit the async code consistently, and it is matching flutter tools scenario (the code I keep hitting there when pausing is part of registering extensions machinery).

Update: added a test in expression_evaluator_test.dart that produces an async frame error. I does not test for the absence of a warning though, as the warning comes from chrome_proxy_service.dart. The previously added evaluation test covers that though.

@annagrin annagrin requested a review from elliette April 6, 2023 22:36
@elliette elliette merged commit 3dee56b into dart-lang:master Apr 7, 2023
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.

[dwds] Error messages appear on hover over in VSCode debug console if the app is paused in JS
2 participants