1278324 - Add linter rule against loading server files from devtools/client

Status: NEW

(DevTools :: General, task, P3)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This has been coming up during the actor decoupling.

Our module requirements should be a dag, where if we simplify/group the module dependency graph to just devtools/server, devtools/shared, and devtools/client, then we should get this DAG (A->B == A requires B):

                    devtools/shared
                         ^     ^
                         |     |
               .---------'     '---------.
               |                         |
               |                         |
    devtools/client            devtools/server

devtools/client should not depend on devtools/server and devtools/server should not depend on devtools/client.

Sometimes when this relationship is broken, the result is a module not being able to be loaded in Fennec (but not desktop). Sometimes everything silently works, entrenching us in future suckiness when untangling everything.

We should define a custom lint to statically assert these requirements.

Comment 1

[Patrick Brosset <:pbro>]

Custom mozilla eslint rules can be found in the mozilla eslint plugin located at:
\testing\eslint\eslint-plugin-mozilla\

The new rule should go in \lib\rules\ and some usage doc should be written in \doc\
The new rule should also be indexed and turned off by default in \lib\index.js

This rule should be configurable. Something like this:

"no-unallowed-require": [
  2,
  [
    {"from": "devtools/client", "to": "devtools/server"},
    {"from": "devtools/server", "to": "devtools/client"}
  ]
]

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

A small aside is at the moment, it's a bit hard to change the rules that run on try because (after bug 1265082) they are uploaded to tooltool which only some people have access to.  :mratcliffe has access on our team, and I am requesting access as well.

We should probably rework this design so the in-tree rules can be changed without updating tooltool, but anyway that's the current state of things.

Anyway, you can at least make local changes and test them locally.  Someone with tooltool access would be needed to update automation.

Comment 3

[Tom Tromey :tromey]

Bug 1264649 added a "reject-some-requires" rule that can do this.
E.g., https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/.eslintrc#10

Comment 4

[Julien Wajsberg [:julienw]]

We just got

Comment 5

[Julian Descottes [:jdescottes]]

(thanks :julienw for the pointer)

We recently added linting rule to prevent modules in devtools/server and devtools/shared from importing devtools/client files.
devtools/server and devtools/shared are always bundled with Firefox while devtools/client is optional.

So what remains to do in this bug would be to prevent module from devtools/client from importing devtools/server files. Since devtools/server is always available, this would not result in a crash. However such an import is most likely wrong. Remember that the real server on the debug target can use a different version of the code. For cleanliness, we should also avoid those imports.

See Bug 1604485 and Bug 1591013 for the shared & server rules.