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

[DWDS] Can't set breakpoint in file with circular dependency #1692

Open
elliette opened this issue Jul 18, 2022 · 8 comments
Open

[DWDS] Can't set breakpoint in file with circular dependency #1692

elliette opened this issue Jul 18, 2022 · 8 comments
Assignees
Labels
P2 A bug or feature request we're likely to work on package:dwds triaged

Comments

@elliette
Copy link
Contributor

elliette commented Jul 18, 2022

See flutter/flutter#106727 for details.

How to reproduce:

  • Download the minimal example
  • Rename lib/main_dev.dart to main.dart
  • Follow instructions to debug DWDS with flutter_tools using the example app
  • Try to set a breakpoint on line 21 of account_presentation/controllers/sign_in_controller.dart

Note:

  • When the import that introduces a circular dependency (package:router/router.dart) is commented out, the moduleForSourcemethod correctly returns packages/account_presentation/controllers/sign_in_controller.dart.
  • With the import is included, it incorrectly returns packages/home/pages/welcome_page.dart.
@elliette
Copy link
Contributor Author

Looked at this a bit more. With the circular dependency, account_presentation/controllers/sign_in_controller.dart gets listed as a library for the packages/home/pages/welcome_page.dart module.

ModuleMetadata for packages/home/pages/welcome_page.dart:

With circular dependency
{
   "version":1.0.2,
   "name":"packages/home/pages/welcome_page.dart",
   "closureName":"load__packages__home__pages__welcome_page_dart",
   "sourceMapUri":"/packages/home/pages/welcome_page.dart.lib.js.map",
   "moduleUri":"/packages/home/pages/welcome_page.dart.lib.js",
   "libraries":[
      {
         "name":"welcome_page",
         "importUri":"package":"home/pages/welcome_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_page",
         "importUri":"package":"home/pages/unknown_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_page",
         "importUri":"package":"home/pages/splash_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"main_layout_page",
         "importUri":"package":"home/layouts/main_layout_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_controller",
         "importUri":"package":"home/controllers/unknown_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_controller",
         "importUri":"package":"home/controllers/splash_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"sign_in_page",
         "importUri":"package":"account_presentation/pages/sign_in_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"sign_in_controller",
         "importUri":"package":"account_presentation/controllers/sign_in_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"sign_in_binding",
         "importUri":"package":"account_presentation/bindings/sign_in_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"account_presentation_library",
         "importUri":"package":"account_presentation/account_presentation_library.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"router",
         "importUri":"package":"router/router.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"router_library",
         "importUri":"package":"router/router_library.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"main_layout_controller",
         "importUri":"package":"home/controllers/main_layout_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"welcome_binding",
         "importUri":"package":"home/bindings/welcome_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_binding",
         "importUri":"package":"home/bindings/unknown_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_binding",
         "importUri":"package":"home/bindings/splash_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"home_library",
         "importUri":"package":"home/home_library.dart",
         "partUris":[
            
         ]
      }
   ],
   "soundNullSafety":true
}
Without circular dependency
{
   "version":1.0.2,
   "name":"packages/home/pages/welcome_page.dart",
   "closureName":"load__packages__home__pages__welcome_page_dart",
   "sourceMapUri":"/packages/home/pages/welcome_page.dart.lib.js.map",
   "moduleUri":"/packages/home/pages/welcome_page.dart.lib.js",
   "libraries":[
      {
         "name":"welcome_page",
         "importUri":"package":"home/pages/welcome_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_page",
         "importUri":"package":"home/pages/unknown_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_page",
         "importUri":"package":"home/pages/splash_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"main_layout_page",
         "importUri":"package":"home/layouts/main_layout_page.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_controller",
         "importUri":"package":"home/controllers/unknown_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_controller",
         "importUri":"package":"home/controllers/splash_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"router",
         "importUri":"package":"router/router.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"router_library",
         "importUri":"package":"router/router_library.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"main_layout_controller",
         "importUri":"package":"home/controllers/main_layout_controller.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"welcome_binding",
         "importUri":"package":"home/bindings/welcome_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"unknown_binding",
         "importUri":"package":"home/bindings/unknown_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"splash_binding",
         "importUri":"package":"home/bindings/splash_binding.dart",
         "partUris":[
            
         ]
      },
      {
         "name":"home_library",
         "importUri":"package":"home/home_library.dart",
         "partUris":[
            
         ]
      }
   ],
   "soundNullSafety":true
}

Adding @annagrin who has more context. How should ModuleMetadata / breakpoints handle circular dependencies?

@annagrin
Copy link
Contributor

Circular dependencies result in JS modules that include all JS code for the dart included, so the module change is expected. Not sure what causes the issue, will look into it.

@annagrin annagrin self-assigned this Jul 19, 2022
@jkydevelopment
Copy link

Hi @annagrin, do you have any news about this? Thanks in advance!

@annagrin
Copy link
Contributor

annagrin commented Aug 6, 2022

Investigated this - the problem is caused by disagreements between serving paths in dwds and relative paths in source maps:

Example

source directory tree

<current directory>
  a/
     lib/
       a.dart
     pubspec.yaml 
  b/
     lib/
       b.dart
     pubspec.yaml

where

  • package:a/a.dart imports package:b/b.dart
  • package:b/b.dart imports package:a/a.dart

DDC output

  • Module:
    packages/a/a.lib.js that contains code for both packages

  • Source map:
    packages/a/a.lib.js.map that contains paths relative to directory a/lib (where .js file is physically located):

    • a.dart
    • ../../b/lib/b.dart
      (meaningless relative to the serving path packages/a/a.lib.js - it should be ../b/b.dart to resolve correctly).

We can solve this by making relative paths in modules uris and relative paths in source maps agree, ie making both match either the actual directory structure or the structure of module uris (which are created from package: urls I believe)

This needs some experimentation and thinking, so not a trivial bug fix unfortunately. I suspect the fix should be in the frontend server since it writes out module metadata (that contains module uris) and source maps (that contain relative source paths), or flutter tools (that reads everything from the frontend server and starts dwds to serve the files).

@jkydevelopment To work around the issue, is it possible to break the circular dependency by either putting all the mutual dependent code in one package, or pulling the code both need to a third package and making the two depend on it instead?

@jkydevelopment
Copy link

@annagrin thank you for your response. I have just tried it by moving some common packages to a new common package, which now contains core, locator and router.

In this case it's not possible to fix the circular dependency because of the dependency injection:

  • locator folder (now contained inside the new common package) uses get_it, so it needs to import some other packages to register each factory or singleton (ie account_domain).
  • In the other hand any other package which uses locator (ie account_domain) also needs to inject some elements which are retrieved from locator, which is inside common package, so the circular dependency it's always present.

The only way I could fix this issue is by only using the main lib folder and not creating any other package, but all the libraries would be contained in the main pubspec.yaml, and it would not be possible to apply a clean architecture to the project.

Any ideas about how could I structure this to avoid the circular dependency?

@annagrin
Copy link
Contributor

annagrin commented Aug 8, 2022

I am working on the solution but not sure when it is going to be available (really depends where the change needs to be made, I am experimenting with a few ideas).

As for the package dependencies, looks like the account_domain is special because it creates a circular dependency. Can the part of account_domain (the part that uses locator and common) be moved to the common package so common does not need to depend on account_domain anymore? Another option is to unify common and account_domain into one package, if this makes more sense for the project.

@annagrin
Copy link
Contributor

Proposed fix

make server paths follow directory structure:packages/<package_directory>/<path from package root to the file on disk>

Fix plan

  • Frontend server and expression compiler worker (DDC):
    • update the module uris in metadata files to match the scheme above, under a new debugger-module-uris flag
  • Dwds:
    • provide a way to map package: uris to server paths, ie a new class PackageUriMapper
    • pass the debugger-module-uris to the expression compiler worker
  • flutter tools:
    • use PackageUriMapper dwds API to resolve package: uris in WebAssetServer
    • pass the debugger-module-uris to the frontend server
  • cleanup:
    • set debugger-module-uris to true by default
    • after all the changes propagated to dart and flutter stable versions
      • remove passing of the flag by tools
      • remove the flag

annagrin pushed a commit to annagrin/flutter that referenced this issue Aug 18, 2022
The flag is true by default so the behavior does not change.

Next steps:

Use the flag for updated debugger module names:
  - Frontend server: make the current behavior controlled
    by the flag non-conditional
  - Frontend server: add more debugging names changes under
    the same flag, false by default
  - Dwds: make changes required for the new module names.
  - Flutter tools: when matching dwds changes roll to flutter,
    pass the flag to the frontend server again.

- Cleanup:
  - Frontend server: make new behavior default
  - Flutter tools: remove uses of the flag.
  - Frontend server: remove the flag.

Towards: dart-lang/webdev#1692
Helps: flutter#106727
annagrin pushed a commit to flutter/flutter that referenced this issue Aug 19, 2022
…09791)

The flag is true by default so the behavior does not change.

Next steps:

Use the flag for updated debugger module names:
  - Frontend server: make the current behavior controlled
    by the flag non-conditional
  - Frontend server: add more debugging names changes under
    the same flag, false by default
  - Dwds: make changes required for the new module names.
  - Flutter tools: when matching dwds changes roll to flutter,
    pass the flag to the frontend server again.

- Cleanup:
  - Frontend server: make new behavior default
  - Flutter tools: remove uses of the flag.
  - Frontend server: remove the flag.

Towards: dart-lang/webdev#1692
Helps: #106727
annagrin pushed a commit to annagrin/webdev that referenced this issue Aug 19, 2022
@bkonyi bkonyi added P2 A bug or feature request we're likely to work on triaged labels Nov 6, 2024
@bkonyi
Copy link
Collaborator

bkonyi commented Nov 6, 2024

We should verify whether or not this is still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on package:dwds triaged
Projects
None yet
Development

No branches or pull requests

5 participants