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

chore: Organise imports #8527

Merged
merged 4 commits into from
Aug 15, 2024
Merged

Conversation

cpcallen
Copy link
Contributor

The basics

The details

Proposed Changes

Use the prettier-plugin-organize-imports npm package to automatically sort imports (and remove unused imports) when formatting code using Prettier.

Reason for Changes

The style guide does not give any definitive instructions about how to organise imports (unlike with goog.requires, for which it previously did).

As a result, our imports have gotten to be somewhat arbitrarily ordered. This shouldn't matter, but it has a few possible consequences:

  • I am a bit unsure of where to add new imports, and this tempts me to waste time manually organising imports.
  • In theory the order of imports should not matter (order of execution of imported modules is arbitrary, so long as all are executed before the importing module) but side effects, Closure Compiler, etc. can all result in practice not strictly agreeing with theory.

For those reasons I think it would be good to automatically order imports and do so in a way consistent with google3, and:

I am not certain that the plugin sorts in the same order as our internal tooling, because I've not been able to find documentation about the expected order for either, but the API does not seem to have much in the way of options (just an option not to remove unused imports) and some manual spot-checking did not unearth any differences. For the same reason it should also be compatible with how e.g. VSCode's organise imports functionality works too.

Additional Information

Observationally, the sort order used by the plugin is to sort by imported path:

  • NPM modules before relative paths.
  • Relative paths starting with ../ before paths starting with ./.
  • import type before plain import, if both exist for the same path.

Named imports are sorted alphabetically, case-insensitive.

Side-effect-only imports (import './path/to/file.js';) appear to be left alone at the top of the import block.

@cpcallen cpcallen added the PR: chore General chores (dependencies, typos, etc) label Aug 14, 2024
@cpcallen cpcallen requested a review from a team as a code owner August 14, 2024 12:54
@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Aug 14, 2024
@cpcallen cpcallen marked this pull request as draft August 14, 2024 13:08
@cpcallen
Copy link
Contributor Author

Just realised that it preserves groupings of imports where separated by blank lines. Seems like a useful feature, but we seem to have some pretty arbitrary and I think in cases quite accidental such groupings, which should probably be rectified.

(This explains why my spot-checking of side-effect-only imports suggested they were left alone: they happened to be in a separate group that was already sorted.)

Converting to draft temporarily while I rectify that.

Since prettier-plugin-organize-imports sorts imports within
sections separated by blank lines, but preserves the section
divisions, remove any blank lines that are not dividing imports
into meaningful sections.

Do not remove blank lines separating side-effect-only imports
from main imports.
@cpcallen cpcallen marked this pull request as ready for review August 15, 2024 00:08
@cpcallen cpcallen merged commit ce22f42 into google:develop Aug 15, 2024
7 checks passed
@cpcallen cpcallen deleted the chore/organise-imports branch August 15, 2024 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants