[flutter_tools] Update to latest dwds APIs #51004

jonahwilliams · 2020-02-19T00:15:23Z

Update to latest dwds APIs, moving back to dwds driven hot restart and enabling future work on expression evaluation.

jonahwilliams · 2020-02-19T00:53:52Z

packages/flutter_tools/lib/src/web/bootstrap.dart

-              return null;
-            }
-          }
+        var module = window.\$requireLoader.urlToModuleId.get(url)


It looks like the dartLoader doesn't exist anymore - should I be using the requireLoader? Otherwise I'm not sure exactly what this code should look like. I tried a few permutations but they all crash

You shouldn't need either. You are correct that the dartLoader was removed and replaced with reqruireLoader. However, package:dwds now embeds the requrieLoader in the entry bootstrap.

See here:
https://github.com/dart-lang/webdev/blob/master/dwds/lib/src/handlers/injected_handler.dart#L74

And here:
https://github.com/dart-lang/webdev/blob/master/dwds/lib/src/loaders/require.dart#L179

Let me know if removing this doesn't fix your problem.

Removing SGTM, but I don't see any handling of the dartStackTraceUtility in the new dwds code

@jakemac53 said that logic was safe to remove. He can probably provide more context.

Another difference is that If I throw an unhandled exception, before the stackTraceMapper would first be called with a url like http://localhost:53973/dart_sdk.js that would be mapped to the module name dart_sdk. Now it's failing with http://localhost:53973/something/dart_stack_trace_mapper.js which might indicate something isn't getting set up correctly. Still investigating

I think it needs to be added by build_web_compilers - it has its own version. Flutter also i think has its own version.

Is there an equivalent to this line I need to do for the requireLoader?

https://github.com/dart-lang/build/blob/8c8ed2b52357b62971889ab2c8c26db2163c0080/build_web_compilers/lib/src/dev_compiler_bootstrap.dart#L366

I'm currrently using the following code:

dart_sdk._isolate_helper.startRootIsolate(() => {}, []); if (window.\$dartStackTraceUtility && !window.\$dartStackTraceUtility.ready) { window.\$dartStackTraceUtility.ready = true; let dart = dart_sdk.dart; window.\$dartStackTraceUtility.setSourceMapProvider(function(url) { url = url.replace(window.\$dartUriBase, ''); var module = window.\$requireLoader.urlToModuleId.get(url); if (!module) return; return dart.getSourceMap(url); }); }

But I'm only get url from the stack trace mapper itself, which leads me to believe that I set it up incorrectly.

There is not but I'm happy to add it to package:dwds somewhere here: https://github.com/dart-lang/webdev/blob/master/dwds/lib/src/loaders/require.dart#L222

Its not so much what API goes into dwds, there is a slight bug now since we use the stack trace mapper from the SDK. This expects there to be a field called window.$dartLoader.rootDirectories:

https://github.com/dart-lang/sdk/blob/master/pkg/dev_compiler/web/stack_trace_mapper.dart#L39

All stack traces actually are crashing in the stack trace mapper now. I can work around by defining this in the bootstrap script myself

Ahh I see. I'm thinking we just duplicate window.$dratLoader.rootDirectories in the DWDS bootstrap for now and eventually clean up the SDK.

jonahwilliams · 2020-02-20T18:06:04Z

Everything seems to working for the most part, except the hot restarts are requesting more resources than necessary and doing full page reloads. I think I've misconfigured something with the modules/digests, but I'm not sure where

jonahwilliams · 2020-02-21T21:45:42Z

So far I've discovered that the special entrypoint in temp seems to be throwing off the module naming. It works a bit better if I stick packages/packagename in front of the module name. Still not reload correctly as client.js can't seem to match the right extension in the $requireLoader urlToModule

grouma · 2020-02-24T18:13:16Z

@annagrin has been running into similar issues while testing. Let's put this on hold for now and I'll dig through the issue with her.

jonahwilliams · 2020-02-24T18:14:46Z

It might help if we could relax the requirement to use either a multiroot scheme or a package scheme. That would at least simplify some of the paths used and server configuration.

packages/flutter_tools/lib/src/build_runner/devfs_web.dart

jonahwilliams · 2020-03-16T20:00:03Z

dev/bots/test.dart

  bool useBuildRunner = false,
  String coverage,
  bq.TabledataResourceApi tableData,
+  bool forceSingleCore = false,


Without this change, we would try to spin up multiple chrome instances and stall. I'm also suspicious that some of the previous flakes in integration tests were caused by having multiple flutter_tester shells active.

This should go in a comment.

This is interesting. Do you not use the concurrency parameter of package:test?

We do, but by default it gets set to the number of cores that the bot has.

yjbanov

LGTM if LGT @grouma

grouma · 2020-03-16T20:23:34Z

dev/bots/test.dart

  bool useBuildRunner = false,
  String coverage,
  bq.TabledataResourceApi tableData,
+  bool forceSingleCore = false,


This is interesting. Do you not use the concurrency parameter of package:test?

grouma · 2020-03-16T20:25:06Z

packages/flutter_tools/lib/src/build_runner/devfs_web.dart

  @override
-  Future<String> dartSourceContents(String serverPath) {
+  Future<String> dartSourceContents(String serverPath) async {
+    if (serverPath == null || serverPath.trim().isEmpty) {


This is an interesting guard. Not that it's wrong by does package:dwds ever request null paths?

I noticed that devtools was requesting null paths before the change to the tracked library names. I will re-verify this

Updated - we only get a null path when devtools tries to request the web-entrypoint file with the org-dartlang-app scheme. I made this check more specific, but we should probably fix/hide this file otherwise

grouma · 2020-03-16T20:26:36Z

packages/flutter_tools/lib/src/web/bootstrap.dart

 document.head.appendChild(requireEl);
-
-// Invoked by connected chrome debugger for hot reload/restart support.
-window.\$hotReloadHook = function(modules) {


Love this cleanup!

grouma · 2020-03-16T20:27:09Z

packages/flutter_tools/lib/src/web/bootstrap.dart

-        return dart.getSourceMap(module);
-      });
-  }
+  dart_sdk._isolate_helper.startRootIsolate(() => {}, []);


I'm not familiar with this. What is this for?

I think I copied this from the build_runner tooling, so its probably just a straggler. Will remove once I verify its not necessary

Not necessary removed

grouma · 2020-03-16T20:27:43Z

packages/flutter_tools/lib/src/web/bootstrap.dart

-// Require JS configuration.
-require.config({
-  waitSeconds: 0,
+window.\$dartLoader = {};


Is the dartLoader stuff required? Shouldn't be anymore.

We still need this since we're using the stack trace mapper from the SDK, and that code unconditionally refers to these getters

Oh yes. Hmmm. We should reconsider that at one point.

grouma · 2020-03-16T20:28:21Z

packages/flutter_tools/test/integration.shard/debugger_stepping_web_test.dart

+    tempDirectory = createResolvedTempDirectorySync('debugger_stepping_test.');
+  });
+
+  test('Web debugger can step over statements', () async {


zanderso · 2020-03-16T20:09:39Z

dev/bots/test.dart

      final String suffix = Platform.isWindows && subshard == 'commands'
        ? 'permeable'
        : '';
+      final bool forceSingleCore = Platform.isLinux && subshard == 'integration';


Why only Linux? (Add a comment explaining)

Added comment

zanderso · 2020-03-16T20:10:02Z

dev/bots/test.dart

  bool useBuildRunner = false,
  String coverage,
  bq.TabledataResourceApi tableData,
+  bool forceSingleCore = false,


This should go in a comment.

zanderso · 2020-03-16T20:10:19Z

dev/bots/test.dart

  } else {
    cpus = 2; // Don't default to 1, otherwise we won't catch race conditions.
  }
+  // Integration tests that depend on external processes like chrome


Oh, nm. I see it here.

zanderso · 2020-03-16T20:13:29Z

packages/flutter/lib/src/foundation/stack_frame.dart

    final bool hasPackage = line.startsWith('package');
    final RegExp parser = hasPackage
-        ? RegExp(r'^(package:.+) (\d+):(\d+)\s+(.+)$')
+        ? RegExp(r'^(package.+) (\d+):(\d+)\s+(.+)$')


Consider adding a comment for this regexp. Separately, where would 'package' not be followed by ':' ?

I filled a bug on it here with more details: #52685

kk. Maybe link to that issue in a comment here.

zanderso · 2020-03-16T20:19:31Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

-        );
      }
+    } on Exception catch (err) {
+      globals.printError(err.toString());


If we can't give any more information here, and it will fail later regardless, should this just be a printTrace() ?

Commented below

zanderso · 2020-03-16T20:20:18Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

      }
+    } on Exception catch (err) {
+      globals.printError(err.toString());
+      return OperationResult(1, err.toString());


Will including the err.toString() in the OperationResult case it to be printed twice?

I've just updated here and above to be fatal errors, so we should print the message with a throwToolExit.

I can't think of a case where we hit a HotRestartException or WipError that we could recover from, so getting people out of the tool is likely better than getting stuck in a loop of prompting the user to "Try again after fixing the above error"

zanderso · 2020-03-16T20:21:11Z

packages/flutter_tools/lib/src/compile.dart

      sdkRoot,
      '--incremental',
      '--target=$targetModel',
+      '--debugger-module-names',


Are there any comments worth adding here about what this flag does?

Added a comment. This is a breaking change flag, so once all of the tooling is updated we can make it the default and remove

Is there an issue we can link to for the migration?

Filled and linked #52693

zanderso · 2020-03-16T20:31:14Z

packages/flutter_tools/test/integration.shard/test_driver.dart

        .toList(),
      workingDirectory: _projectFolder.path,
-      environment: <String, String>{'FLUTTER_TEST': 'true'},
+      environment: <String, String>{'FLUTTER_TEST': 'true', 'FLUTTER_WEB': 'true'},


Consider documenting here that this achieves the effect of flutter config --enable-web.

…o devtools_stuff

zanderso

lgtm. when this makes it to dev/, monitor crash logging for new crashers.

lock · 2020-04-02T15:40:18Z

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

jonahwilliams added 2 commits February 18, 2020 15:53

[update to latest dwds

000952d

Try to get sourcemaps working

ee23bd4

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Feb 19, 2020

googlebot added the cla: yes label Feb 19, 2020

jonahwilliams changed the title ~~Devtools stuff~~ [flutter_tools] Update to latest dwds APIs Feb 19, 2020

jonahwilliams commented Feb 19, 2020

View reviewed changes

jonahwilliams added 7 commits February 18, 2020 17:36

more pending work

4371565

Merge branch 'master' of github.com:flutter/flutter into devtools_stuff

8be1de3

more work

6ebff9f

make stack trace mapping work

c4e4282

more work

8edb19e

fix analysis errors

25adfe7

use module list to update

e429c92

jonahwilliams added 2 commits February 20, 2020 14:47

more werk

aab239e

more werk

84fc10c

Merge branch 'master' of github.com:flutter/flutter into devtools_stuff

ea055df

jonahwilliams force-pushed the devtools_stuff branch from 99259cf to ea055df Compare March 8, 2020 18:46

Jonah Williams added 2 commits March 8, 2020 11:51

Update compile.dart

73c1635

back to dwds

1803aec

jonahwilliams commented Mar 9, 2020

View reviewed changes

packages/flutter_tools/lib/src/build_runner/devfs_web.dart Outdated Show resolved Hide resolved

jonahwilliams added 3 commits March 10, 2020 13:11

Merge branch 'master' of github.com:flutter/flutter into devtools_stuff

3a0a661

more work

78e9ddd

update stepping test to add web version

97e2ed6

jonahwilliams commented Mar 12, 2020

View reviewed changes

packages/flutter_tools/lib/src/build_runner/devfs_web.dart Outdated Show resolved Hide resolved

Update binding_test.dart

a151c0f

jonahwilliams requested review from yjbanov and zanderso March 16, 2020 19:58

jonahwilliams commented Mar 16, 2020

View reviewed changes

yjbanov approved these changes Mar 16, 2020

View reviewed changes

grouma approved these changes Mar 16, 2020

View reviewed changes

zanderso reviewed Mar 16, 2020

View reviewed changes

jonahwilliams added 7 commits March 16, 2020 13:56

address comments

0484aa8

add comment explaining debugger module names

62bf801

Update test.dart

c5562dc

Update test_driver.dart

75e4442

make wip and exception fatal errors

36eea80

Update resident_web_runner.dart

b97c7c8

Merge branch 'devtools_stuff' of github.com:jonahwilliams/flutter int…

3df1db9

…o devtools_stuff

jonahwilliams mentioned this pull request Mar 16, 2020

Remove --debugger-module-names flag from frontend_server/tool once all tooling is updated #52693

Closed

Jonah Williams added 2 commits March 16, 2020 14:38

Update compile.dart

34498fa

Update stack_frame.dart

b9591d1

jonahwilliams requested a review from zanderso March 16, 2020 22:34

johnpryan mentioned this pull request Mar 17, 2020

Exception using DevTools with dwds version of Flutter SDK dart-lang/webdev#926

Closed

jonahwilliams requested review from zanderso and removed request for zanderso March 17, 2020 22:03

zanderso approved these changes Mar 17, 2020

View reviewed changes

jonahwilliams added 2 commits March 17, 2020 15:20

Merge branch 'master' of github.com:flutter/flutter into devtools_stuff

399c93b

listen to isolate changes

c478683

jonahwilliams merged commit 6884086 into flutter:master Mar 18, 2020

jonahwilliams deleted the devtools_stuff branch March 18, 2020 00:29

This was referenced Mar 20, 2020

Non-exception breakpoints not being hit In VSCode #52890

Closed

Debugger broken after flutter upgrade #51853

Closed

lock bot locked and limited conversation to collaborators Apr 2, 2020

[flutter_tools] Update to latest dwds APIs #51004

[flutter_tools] Update to latest dwds APIs #51004

Conversation

jonahwilliams commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Feb 20, 2020

Uh oh!

jonahwilliams commented Feb 21, 2020

Uh oh!

grouma commented Feb 24, 2020

Uh oh!

jonahwilliams commented Feb 24, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jonahwilliams commented Feb 19, 2020 •

edited

Loading