1774367 - Add linting rule to check the order of imports in remote/ modules

Status: NEW

(Remote Protocol :: Agent, task, P3)

Description

[Julian Descottes [:jdescottes]]

See the discussion at https://phabricator.services.mozilla.com/D149015#inline-820400

Modules in remote aim to use the following pattern to import modules:

• ChromeUtils.import for modules outside of /remote, sorted alphabetically by module path
• ChromeUtils.import for modules in /remote, also sorted alphabetically by module path
• XPCOMUtils.defineLazyModuleGetters, sorted alphabetically by imported symbol
• XPCOMUtils.defineLazyGetter, sorted alphabetically by getter name

We have no linting to enforce this, so we manually check this at review time.
The goal of this bug is to create an eslint rule which can check the order of our imports.

Comment 1

[Julian Descottes [:jdescottes]]

Added to triage to discuss the overall topic of adding more linting rules to reduce the amount of code style review comments.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Blocks a P1 M5 bug, which means we have to also bump the priority of this bug. I'll keep the already assigned points.

Comment 3

[Julian Descottes [:jdescottes]]

Hi Mark,

We want to add a rule to validate the order in which we import modules in the remote/ folder (for consistency, no strong technical reason behind this).

But this rule will most likely be quite specific to the "remote" codebase, because the main idea is to import first non-remote modules, and then remote modules. Within each group, imports are then sorted alphabetically. Eg if we take the example of a lazyModuleGetters:

XPCOMUtils.defineLazyModuleGetters(lazy, {
  clearInterval: "resource://gre/modules/Timer.jsm",
  setInterval: "resource://gre/modules/Timer.jsm",

  assert: "chrome://remote/content/shared/webdriver/Assert.jsm",
  Log: "chrome://remote/content/shared/Log.jsm",
});

Can I still add such a rule to tools/lint/eslint/eslint-plugin-mozilla/lib/rules, or should it be located in another folder?

Another option is to make the rule configurable enough to avoid hardcoding anything related to "remote". For instance we could have a "groups" option which would be set to ["*", "chrome://remote"] for our use case. Do you think this is worth doing or we should rather do a remote-specific rule here?

Also a side question regarding alphabetical sorting for imports. We noticed that the ESM-ification scripts are automatically sorting imports in a case sensitive way (meaning that in my example above "Log" would have to move before "assert"). Usually in remote we have been sorting in a case insensitive manner. Are there any plans to enforce case sensitive order for imports in the overall codebase?

Comment 4

[Mark Banner (:standard8)]

So generally I would like to minimise specialisms we have across the tree. At some point in the next year or two ESLint's config system is changing to a single-top level file (although we'll probably be able to do imports into it), and hence having custom pieces on many areas of the tree is going to make that difficult to maintain and manage - as well as leading to inconsistencies that may frustrate developers.

Having said that, there are some areas where I think we will still need differences for special reasons. Though I'm not sure this is one of them.

On the subject of sorting imports, I don't think we've had discussions on this within JavaScript land. There is bug 1679522 for C++ and bug 1790816 for Python, which whilst neither have fully landed, I think that gives some indication that it may be desirable.

I did spot the other day, that there is an ESLint rule for imports that I think is very similar to what you're looking at here. There's options for alphabetical sorting, newlines in-between groups, as well as grouping depending on the path. I haven't yet looked to see how the grouping would work with our chrome URIs, but it might?

That rule also sorts alphabetically according to the path rather than the symbol, so that might be something to consider as well.

Personally, I think at least doing alphabetical ordering is reasonable - its something I try to do when writing imports anyway. Grouping according to types might also help - e.g. chrome://gre/modules/ vs chrome://modules/. Though hopefully at some stage we might head more towards directory-based URIs.

If we can implement something that works similarly the import/order rule (and configure that rule to work the same), then I think that would give us nice consistency across the two. Additionally, as hopefully in future we'll replace our lazy imports with something that's ES module supported, then we may again be able to keep the consistency.

I think maybe if we could spec something out perhaps based on the import rule (as that's already implemented), that we could pass it around the desktop leads & module owners and see what they think (a quick first stop might be checking in with :Mossop and :Gijs).

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for the feedback Mark!

If we want to make that a mozilla-central rule, that sounds good to me as well. The way I understand it, I should propose something based on import/order, but with groups support which can match what I' m trying to achieve here, and support for lazy imports? Lazy imports are 95% of our imports so I'm quite interested in having those linted from the beginning, without waiting for a standardized version.

Having said that, there are some areas where I think we will still need differences for special reasons.

What do you think in general of rules which enforce more constraints but which remain compatible with the m-c linter configuration?

In remote/ we have a lot of "hidden" style rules: sorting properties, sorting methods in classes etc, not destructuring arguments.... They are all enforced by review comments which is frustrating and error prone.

I would love to reduce the number of style nits in reviews, but of course that means adding more eslint rules. I initially filed a meta for this at https://bugzilla.mozilla.org/show_bug.cgi?id=1774370. I'm not sure it's reasonable to expect consensus about all those style nits across m-c. Or should we aim for much stricter linting that really covers most of the aspects of the code we write? In the current state, there are many areas where the linter has no opinion which leaves it up to each team to define (or not) their conventions.

I guess that even with rules which only "add" to the default configuration, there is still the risk that two teams might use orthogonal rules which would be frustrating. So maybe increasing the surfaces of our shared linting rules is the only solution to reduce codestyle nits?

Comment 6

[Mark Banner (:standard8)]

(In reply to Julian Descottes [:jdescottes] from comment #5)

If we want to make that a mozilla-central rule, that sounds good to me as well. The way I understand it, I should propose something based on import/order, but with groups support which can match what I' m trying to achieve here, and support for lazy imports? Lazy imports are 95% of our imports so I'm quite interested in having those linted from the beginning, without waiting for a standardized version.

I think my ideal would be being able to configure import/order and the new lazy import rule in exactly the same way as this would lead to consistency if we enable both the rules (without having to write our own custom version for import).

I didn't mean to imply we have to wait for standardised versions - I'm quite happy to implement something now, I'm just thinking that if we're able to use the same configuration for both, then it might help for when we do get a standardised lazy version. I have just asked a few people about what they think of requiring sorting (waiting to hear back).

I guess that even with rules which only "add" to the default configuration, there is still the risk that two teams might use orthogonal rules which would be frustrating. So maybe increasing the surfaces of our shared linting rules is the only solution to reduce codestyle nits?

I'm totally happy with the idea of reducing code-style nits for reviewers. That's one of the reasons FF Hello adopted ESLint in the first place.

I think at a minimum I would aim for consistency - e.g. if one or more teams want sort orders for arrays, then they should be consistent - but there's also maybe a discussion to be had on how far we go or how much we want the code to be consistent across the whole repository. We've not had that sort of discussion yet, but maybe we're getting to the point where we should have it and form some guidelines.

Comment 7

[Julian Descottes [:jdescottes]]

Looks like there is some interest to get an import sorting rule working across all JS modules. We should decide a few key points:

• do we group similar imports together (eg toolkit first, then non-toolkit)
• do we sortby import symbol or by path (taking into consideration that synchronous imports can import several symbols from the same path on a single line)

Will start a document to lay out options and gather feedback.