Move inspector_service.dart to root of shared.

[polina-c]

RELEASE_NOTE_EXCEPTION=[refactoring]

Follow up for https://github.com/flutter/devtools/pull/4991/files/e16dbbba3ff0b40c878377ac587f3c8f2e79b4f5#r1057805952.

No functional changes.

[kenzieschmoll]

I think it makes the most logical sense to have the inspector_service.dart file live under inspector/. We don't have to follow a strict practice of preventing any one area of DevTools from importing some part of another screen/ directory. IMO it would be more confusing that something that is called "inspector_service" does not live under inspector/

[polina-c]

I think it makes the most logical sense to have the inspector_service.dart file live under inspector/. We don't have to follow a strict practice of preventing any one area of DevTools from importing some part of another screen/ directory. IMO it would be more confusing that something that is called "inspector_service" does not live under inspector/

I suggest to introduce this practice, except cases when it means significant overhead.

The folder shared contains code that is designed to be shared between screens, and having inspector_service under shared would mean exactly this: the code is shared between screens. I.e. that inspector_service is not dedicated to serve just inspector screen. Other screens are using it too and whoever modifies it should be more careful. And opposite: the code under screen/x is used by screen x only and it is easy to see what will be affected by modifications.

How does it sound?

[kenzieschmoll]

I think it makes the most logical sense to have the inspector_service.dart file live under inspector/. We don't have to follow a strict practice of preventing any one area of DevTools from importing some part of another screen/ directory. IMO it would be more confusing that something that is called "inspector_service" does not live under inspector/

I suggest to introduce this practice, except cases when it means significant overhead.

The folder shared contains code that is designed to be shared between screens, and having inspector_service under shared would mean exactly this: the code is shared between screens. I.e. that inspector_service is not dedicated to serve just inspector screen. Other screens are using it too and whoever modifies it should be more careful. And opposite: the code under screen/x is used by screen x only and it is easy to see what will be affected by modifications.

How does it sound?

For the case of the inspector_service.dart file, I can see the argument to put this in shared/, given how many different places across DevTools it is used, and given that it does not import any other libraries from screens/inspector/. For general purpose items (utils, service managers, analytics, etc.), following a practice of storing in shared/ also makes sense.

In general though, I don't think pulling "screen-specific" code out of its respective screens/ folder into shared/ is something we should enforce. We should look at this on a case-by-case basis and see if the number of uses of a library outside of its screens/ folder warrants moving to shared/ (maybe arbitrarily more than 2-3 uses?). Another factor in this decision should be how many other libraries under screens/some_screen/ would have to be moved to shared/ in order to enforce this layering rule. Enforcing this pattern could lead us into a situation where it is difficult to find all the code for a feature or screen because it is split between screens/ and shared/.

Some examples where it is reasonable to have cross-screen imports:

• other screens may need to access the debugger controller, but would be a bit confusing to have the debugger controller code live outside of the debugger/ directory.
• the performance/ screen uses elements from the cpu_profiler/ screen, and that is okay.

My general opinion is this:

• we should place "screen-specific" items in shared/ only where a strong argument can be made that this is truly a "cross-devtools" component and not just a library with a couple select uses outside of its screens/ directory.
• layering guidelines or "best practices" are good for overall code health, but strict layering rules brings more overhead than benefit, especially for a package like devtools_app, which is private to our repo, and not consumed by any other packages in the dart/flutter ecosystem.

[polina-c]

I think it makes the most logical sense to have the inspector_service.dart file live under inspector/. We don't have to follow a strict practice of preventing any one area of DevTools from importing some part of another screen/ directory. IMO it would be more confusing that something that is called "inspector_service" does not live under inspector/

I suggest to introduce this practice, except cases when it means significant overhead.
The folder shared contains code that is designed to be shared between screens, and having inspector_service under shared would mean exactly this: the code is shared between screens. I.e. that inspector_service is not dedicated to serve just inspector screen. Other screens are using it too and whoever modifies it should be more careful. And opposite: the code under screen/x is used by screen x only and it is easy to see what will be affected by modifications.
How does it sound?

For the case of the inspector_service.dart file, I can see the argument to put this in shared/, given how many different places across DevTools it is used, and given that it does not import any other libraries from screens/inspector/. For general purpose items (utils, service managers, analytics, etc.), following a practice of storing in shared/ also makes sense.

In general though, I don't think pulling "screen-specific" code out of its respective screens/ folder into shared/ is something we should enforce. We should look at this on a case-by-case basis and see if the number of uses of a library outside of its screens/ folder warrants moving to shared/ (maybe arbitrarily more than 2-3 uses?). Another factor in this decision should be how many other libraries under screens/some_screen/ would have to be moved to shared/ in order to enforce this layering rule. Enforcing this pattern could lead us into a situation where it is difficult to find all the code for a feature or screen because it is split between screens/ and shared/.

Some examples where it is reasonable to have cross-screen imports:

• other screens may need to access the debugger controller, but would be a bit confusing to have the debugger controller code live outside of the debugger/ directory.
• the performance/ screen uses elements from the cpu_profiler/ screen, and that is okay.

My general opinion is this:

• we should place "screen-specific" items in shared/ only where a strong argument can be made that this is truly a "cross-devtools" component and not just a library with a couple select uses outside of its screens/ directory.
• layering guidelines or "best practices" are good for overall code health, but strict layering rules brings more overhead than benefit, especially for a package like devtools_app, which is private to our repo, and not consumed by any other packages in the dart/flutter ecosystem.

Thanks. I agree to discuss this case by case.