chore(eslint): Enforce import order

[scinos]

EXPERIMENT, DO NOT MERGE

Changes proposed in this Pull Request

• Turn off wpcalypso/import-docblock for packages. No more mandatory /* External dependencies */ comment blocks.
• Remove all usages of /** * External dependencies */, /** * Internal dependencies */ and similar blocks
• Turn on import/order
• Apply auto-fix for import/order

The changes were introduced using automated tools:

1. Search instances of /**\n* External dependencies\n*/ and replace it by an empty line. Same for Internal dependencies, WordPress dependencies, Module dependencies and Type dependencies.
2. Run prettier to get rid of the extra new lines.
3. Enable import/order in .eslintrc.js and apply the auto-fix.

Next steps

If we like it, decide how to apply this to the repo. We can do one massive PR for all the repo, many small PRs (eg: one per package/dir), or anything in between.

Alternative: there is an improved version of the rule in Gutenberg: https://github.com/WordPress/gutenberg/blob/trunk/packages/eslint-plugin/rules/dependency-group.js. We could adopt that as well. I'm not sure if it will work together with import/order to enforce a specific order inside each group.

[github-actions]

Test live: https://calypso.live/?branch=try/enforce-import-order

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~77 bytes removed 📉 [gzipped])

name                   parsed_size           gzip_size
entry-main                   +34 B  (+0.0%)       -2 B  (-0.0%)
entry-login                  +34 B  (+0.0%)       +3 B  (+0.0%)
entry-domains-landing        +28 B  (+0.0%)      +17 B  (+0.0%)
entry-gutenboarding          +26 B  (+0.0%)     -100 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~78 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
checkout        -14 B  (-0.0%)      +78 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~56 bytes added 📈 [gzipped])

name                                             parsed_size           gzip_size
async-load-signup-steps-design-picker                  -14 B  (-0.0%)      -10 B  (-0.0%)
async-load-calypso-blocks-editor-checkout-modal        -14 B  (-0.0%)      +66 B  (+0.0%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[noahtallen]

I personally liked having internal/external dependencies separated since it seemed more organized. I also personally don't think that import order is very important in terms of DevX. I wonder what others think though -- I don't know any practical reasons to go either way.

[scinos]

My opinion about these rules:

I don’t really care about the import order, when I search for a package I either cmd+click on the usage to follow the import, or cmd+f to find them by name. I don’t get any value from imports being sorted or grouped. This applies to both rules, of course.

What I don’t like is the wpcalypso/import-docblock rule, for a few reasons:

• It forces you to write the bad kind of comments: document what is already in expressed in the code (eg: I don’t get any value from a comment saying “internal dependencies”, I can already see they import things using a relative path)
• It doesn’t actually check that your “internal dependencies” are, in fact, internal, the comment can get out of sync (to be fair, this has been fixed in the Gutenberg version).
• It doesn’t have auto-fix, so I have to manually type the doc block over and over again (also fixed in the Gutenberg version).
• It reports the whole file as ‘failing’ potentially covering more useful and relevant eslint errors until I type the docblock.

My guess is that wpcalypso/import-docblock was useful when we had package-relative imports (eg: import foo from 'state/foo') as you don't know if state is an external dependency or a directory inside ./client. But that pattern is gone now. Looking at the origin of this rule in Gutenberg (WordPress/gutenberg#716), looks like that was the original intention.

[tyxla]

Thanks for bringing this up, @scinos.

Personally, I don’t care much about the docblocks, as long as we’re consistent throughout the codebase. I personally used to see some value in splitting the imports to internal and external, as it made things more readable, but at the same time, more and more of the repo code is in packages now, so it all ends up being external anyway…

Regarding sorted imports, for my brain this improves readability and I prefer having it, it does make the code more readable in my opinion - especially if you scan through code like you diagonally read through a page of a book (that's what I often do). I’ve preferred sorting the imports in my code, but I’m not sure it will be as preferred by all calypso devs, thus I’m not sure enforcing a rule for that would make sense.

In any case, this is worth trying out IMHO but will have to be very short-lived PR.

[jsnajdr]

I like the import/order sorting more. The docblocks don't add any value subjectively, and I've never seen anything similar used outside the Calypso/Gutenberg universe.

I think it's valuable though to be aligned with Gutenberg standards. Consider proposing the same change there.

Personally, I feel that import ordering affects my devex very little. It just doesn't matter much. But often I find myself instinctively reordering imports in PRs so that local relative ones (from './foo') are at the end and that same directories (e.g., from 'calypso/state/...') are grouped together. So I can't honestly say I totally don't care 🙂

[github-actions]

This PR has been marked as stale due to lack of activity within the last 30 days.

[scinos]

Being done in #54448