Skip to content

Enable expression evaluation in debugger for web platform #53595

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

Jump to bottom
Merged
merged 10 commits into from
Apr 2, 2020
Merged

Enable expression evaluation in debugger for web platform #53595

merged 10 commits into from
Apr 2, 2020

Conversation

annagrin
Copy link
Contributor

Description

  • Added ResidentCompiler.compileExpressionToJs API that calls matching API from frontend server in the SDK.
  • Added WebExpressionCompiler class and passed its instance to DWDS to enable expression evaluation in the debugger for web platform
  • Added integration tests for expression evaluation in the debugger
  • Updated mocks to implement new API

Related Issues

Closes #51669

Tests

I added the following tests:

test/integration.shard/expression_evaluation_web_test.dart

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I signed the CLA.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I updated/added relevant documentation (doc comments with ///).
  • All existing and new tests are passing.
  • The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

  • No, no existing tests failed, so this is not a breaking change.
  • Yes, this is a breaking change. If not, delete the remainder of this section.
    • I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
    • I got input from the developer relations team, specifically from: Replace with the names of who gave advice
    • I wrote a migration guide: Replace with a link to your migration guide

@fluttergithubbot fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Mar 30, 2020
jakemac53
jakemac53 reviewed Mar 30, 2020
View reviewed changes
jonahwilliams
jonahwilliams reviewed Mar 30, 2020
View reviewed changes
@jonahwilliams
Copy link
Member

There is currently an analysis warning with this change:

▌23:27:05▐ RUNNING: cd .; bin/flutter analyze --dartdocs --flutter-repo
Analyzing 3 directories...                                      

warning • The parameter 'expressionCompiler' is required • packages/flutter_tools/test/general.shard/web/devfs_web_test.dart:367:31 • missing_required_param

1 issue found. (ran in 270.3s)

For the integration test failures, I'll take a look tomorrow to see If I can narrow down the issue

@jonahwilliams
Copy link
Member

You also need to pull in 9450007 so that the windows template test passes

@jonahwilliams
Copy link
Member

I ran these tests a few times locally and they passed - so it might just be too expensive for CI. I would update the tests to be skipped and I will file an issue to move them into the devicelab, where they can run on dedicated hardware

@annagrin
Copy link
Contributor Author

annagrin commented Apr 1, 2020

Thanks @jonahwilliams! I will disable them then.

@jonahwilliams
Copy link
Member

Filled #53779

jonahwilliams
jonahwilliams approved these changes Apr 1, 2020
View reviewed changes
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you @annagrin !

@annagrin
Copy link
Contributor Author

annagrin commented Apr 1, 2020

Thanks @jonahwilliams for help!

@fluttergithubbot fluttergithubbot merged commit 3a0d837 into flutter:master Apr 2, 2020
@annagrin annagrin deleted the annagrin/eval branch April 22, 2020 19:47
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jakemac53 jakemac53 jakemac53 approved these changes

@grouma grouma grouma approved these changes

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Assignees
No one assigned
Labels
c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add evaluation support to flutter tools for web target
6 participants
@annagrin @jonahwilliams @jakemac53 @grouma @googlebot @fluttergithubbot