Add prettier plugin to automatically order imports #5237

acelaya · 2023-02-09T15:10:10Z

This is an alternative approach to what was proposed in #5231

It serves the same purpose, automating the ordering of imports, but via prettier instead of eslint, as the kind of task fits better with prettier's scope.

Based on the discussions from the original PR, we will have just two groups of imports, separated by an empty line. Each group is ordered alphabetically:

external imports
internal imports

import { ... } from '@hypothesis/...';
import { ... } from 'classnames';
import { ... } from 'foo';

import { ... } from '../../../aaa';
import { ... } from '../../../bbb';
import { ... } from '../../../ccc';
import { ... } from '../../aaa';
import { ... } from '../../bbb';
import { ... } from '../../ccc';
import { ... } from '../aaa';
import { ... } from '../bbb';
import { ... } from '../ccc';
import { ... } from './aaa';
import { ... } from './bbb';
import { ... } from './ccc';

The plugin used for this is https://github.com/trivago/prettier-plugin-sort-imports. I have explored others but this one seems to be the one with higher adoption and better maintained.

~~Again, I have applied the changes only to a couple of files, as a POC.~~ I have applied it to all files in order to check if anything breaks.

acelaya · 2023-02-09T15:12:02Z

package.json

@@ -102,7 +103,10 @@
  "browserslist": "chrome 70, firefox 70, safari 11.1",
  "prettier": {
    "arrowParens": "avoid",
-    "singleQuote": true
+    "singleQuote": true,
+    "importOrder": ["^[./]"],


This plugin considers a third party library to everyimport not matching what's defined here.

With this regular explresion we are telling prettier to create one group for third parties, and another group for everything starting with . or /.

We could add more granular regular expressions here and the plugin would be able to create more groups if desired.

acelaya · 2023-02-09T15:12:57Z

package.json

-    "singleQuote": true
+    "singleQuote": true,
+    "importOrder": ["^[./]"],
+    "importOrderSeparation": true,


This tells the plugin to add an empty line between every group.

acelaya · 2023-02-09T15:19:23Z

yarn.lock

+    debug "^4.1.0"
+    gensync "^1.0.0-beta.2"
+    json5 "^2.1.2"
+    semver "^6.3.0"


This is what I like the least about this plugin. They depend on a bunch of babel packages.

Since we're already depending on Babel, that's not too big of an issue. More disk space if there are more versions being used, but it doesn't expand the surface area of maintainers we need to trust.

robertknight

Looks like a reasonable approach to me. Do you want to try running it over the whole codebase and check for issues with entry points etc?

acelaya · 2023-02-09T15:29:04Z

Looks like a reasonable approach to me. Do you want to try running it over the whole codebase and check for issues with entry points etc?

Let's give it a try

acelaya · 2023-02-09T15:48:36Z

src/sidebar/index.js

+// The entry point component for the app.
+import { render } from 'preact';
+// Enable debugging checks for Preact. Removed in prod builds by Rollup config.
+import 'preact/debug';


@robertknight do we need preact/debug to be imported before preact?

preact/debug imports preact in order to install various options hooks , so I don't think this should cause any problems.

acelaya · 2023-02-09T15:54:20Z

@robertknight I have run make format on the whole codebase, and the only entry point with significant changes is the sidebar one: https://github.com/hypothesis/client/pull/5237/files#diff-298d8680bec67cc6f6533656f41e421ae3c047d046ada2c7ba6271199fc51e60L1

However, I'm testing it locally and doesn't seem to have any evident regressions.

robertknight

This looks good to me from an initial skim through. I had a query about whether it is possible to preserve blank lines above commented imports. This is just a nice-to-have and we can do without it if that is not possible. Please have a look and then I think we could land this and give it a try. There isn't too much conflicting work happening at the moment, so now is a good time.

robertknight · 2023-02-09T16:20:01Z

src/sidebar/index.js

-import { parseJsonConfig } from '../boot/parse-json-config';
+// Load polyfill for :focus-visible pseudo-class.
+import 'focus-visible';
+// The entry point component for the app.


Can we configure the plugin not to remove the new blank lines above commented imports?

Let me check if this can be done.

robertknight · 2023-02-09T16:20:52Z

src/sidebar/index.js

 import {
  startServer as startRPCServer,
  preStartServer as preStartRPCServer,
 } from './cross-origin-rpc.js';
+import { ServiceContext } from './service-context';
+import { AnnotationActivityService } from './services/annotation-activity';
+// Services.


This comment is now out of place. It should be above the previous line. Given that all the service imports come from a directory named ./services it isn't that important though. We could just remove it. Same for the "Utilities" section indicator.

robertknight · 2023-02-09T16:21:47Z

src/sidebar/index.js

+import { TagsService } from './services/tags';
+import { ThreadsService } from './services/threads';
+import { ToastMessengerService } from './services/toast-messenger';
+// Redux store.


This comment can just be removed.

robertknight · 2023-02-09T16:22:46Z

src/sidebar/test/bootstrap.js

+import { patch } from '../../test-util/assert-methods';
+
+// Expose the sinon assertions.
+sinon.assert.expose(assert, { prefix: null });


This is better, as the new ordering matches the actual execution order.

acelaya · 2023-02-09T17:04:02Z

This looks good to me from an initial skim through. I had a query about whether it is possible to preserve blank lines above commented imports. This is just a nice-to-have and we can do without it if that is not possible. Please have a look and then I think we could land this and give it a try. There isn't too much conflicting work happening at the moment, so now is a good time.

@robertknight unfortunately I don't see any option to keep blank lines before commented imports. I would even say this is by design https://github.com/trivago/prettier-plugin-sort-imports/blob/master/docs/TROUBLESHOOTING.md#q-how-does-the-plugin-handle-the-first-comment-in-the-file

I have fixed/removed the incorrect comments from sidebar's index file.

codecov · 2023-02-09T17:05:29Z

Codecov Report

Merging #5237 (9ad9fe3) into main (22e24f5) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5237   +/-   ##
=======================================
  Coverage   99.18%   99.18%           
=======================================
  Files         233      233           
  Lines        9308     9308           
  Branches     2236     2236           
=======================================
  Hits         9232     9232           
  Misses         76       76

Impacted Files	Coverage Δ
src/annotator/adder.tsx	`98.19% <ø> (ø)`
src/annotator/anchoring/html.ts	`97.95% <ø> (ø)`
src/annotator/anchoring/pdf.ts	`100.00% <ø> (ø)`
src/annotator/anchoring/types.js	`100.00% <ø> (ø)`
src/annotator/bucket-bar-client.js	`100.00% <ø> (ø)`
src/annotator/components/AdderToolbar.tsx	`97.14% <ø> (ø)`
src/annotator/components/ContentInfoBanner.js	`100.00% <ø> (ø)`
src/annotator/components/NotebookModal.tsx	`100.00% <ø> (ø)`
src/annotator/components/Toolbar.tsx	`94.11% <ø> (ø)`
src/annotator/config/index.js	`100.00% <ø> (ø)`
... and 80 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

robertknight · 2023-02-10T09:00:41Z

This plugin unfortunately causes Yarn to spit out a warning:

yarn install v1.22.19
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
warning " > @trivago/prettier-plugin-sort-imports@4.0.0" has unmet peer dependency "@vue/compiler-sfc@3.x".
[4/4] 🔨  Building fresh packages...

We definitely don't want to add this peer dependency, since we don't use Vue at all. Can we find a way to suppress the warning though.

Upstream issue: trivago/prettier-plugin-sort-imports#201.

Edit: It looks like this dependency is declared optional in package.json at https://github.com/trivago/prettier-plugin-sort-imports/blob/89d66f706423e44f29d525529af37e5d41a74133/package.json#L61. There might be an issue that the version of Yarn we are using doesn't support this field though.

Edit 2: It looks like the change to mark this dependency as optional was only recently committed upstream, and hasn't been published yet. See trivago/prettier-plugin-sort-imports@a5343b5.

acelaya · 2023-02-10T09:46:07Z

scripts/stub-package/package.json

+  "keywords": [],
+  "author": "",
+  "license": "ISC"
+}


This is a stub package to "trick" yarn into thinking @vue/compiler-sfc is installed.

That package is a peer optional dependency from @trivago/prettier-plugin-sort-imports, however, there's no public release where it is properly marked as optional.

Once they publish a new version including this metadata, we can get rid of this file and its reference in package.json

acelaya · 2023-02-10T09:48:07Z

This plugin unfortunately causes Yarn to spit out a warning:

This should cover it until they publish a release marking that as optional https://github.com/hypothesis/client/pull/5237/files#r1102524594

robertknight

Just a few comments, otherwise I think we can land this. The stub package hack is unfortunate, and I'd like to get that resolved before rolling out to more repos if we can.

robertknight · 2023-02-10T09:09:13Z

src/sidebar/index.js

-import { parseJsonConfig } from '../boot/parse-json-config';
+// Load polyfill for :focus-visible pseudo-class.
+import 'focus-visible';
+// The entry point component for the app.


The "The entry point component" comment is now out of place. I suggest we just remove it.

robertknight · 2023-02-10T09:11:55Z

src/boot/index.js

-import { bootHypothesisClient, bootSidebarApp } from './boot';
-import { processUrlTemplate } from './url-template';
-import { isBrowserSupported } from './browser-check';
-
 // @ts-ignore - This file is generated before the boot bundle is built.


Can you add a blank comment line above this // @ts-ignore. The formatting plugin will delete any purely blank lines, but it will keep lines that start with just //.

robertknight · 2023-02-10T09:47:13Z

package.json

+    "singleQuote": true,
+    "importOrder": ["^[./]"],
+    "importOrderSeparation": true,
+    "importOrderCaseInsensitive": true


I would be inclined to leave this at the default false value, just because that lines up with what happens when eg. sorting identifiers in an object using an editor's sort command.

acelaya · 2023-02-10T10:04:36Z

Just a few comments, otherwise I think we can land this.

All comments addressed

The stub package hack is unfortunate, and I'd like to get that resolved before rolling out to more repos if we can.

Agree

robertknight

LGTM.

…rt-imports peer dependency

acelaya commented Feb 9, 2023

View reviewed changes

acelaya requested review from lyzadanger and robertknight February 9, 2023 15:13

acelaya commented Feb 9, 2023

View reviewed changes

robertknight reviewed Feb 9, 2023

View reviewed changes

acelaya force-pushed the prettier-imports branch from 2bc2af7 to cbcafa6 Compare February 9, 2023 15:36

acelaya commented Feb 9, 2023

View reviewed changes

robertknight reviewed Feb 9, 2023

View reviewed changes

acelaya force-pushed the prettier-imports branch from cbcafa6 to 056d62f Compare February 9, 2023 17:02

acelaya marked this pull request as ready for review February 9, 2023 17:04

acelaya requested a review from robertknight February 9, 2023 17:04

acelaya mentioned this pull request Feb 9, 2023

Add eslint rule to enforce import blocks as we want them #5231

Closed

acelaya commented Feb 10, 2023

View reviewed changes

robertknight reviewed Feb 10, 2023

View reviewed changes

acelaya force-pushed the prettier-imports branch from 7b35049 to 77fc596 Compare February 10, 2023 10:04

acelaya requested a review from robertknight February 10, 2023 10:05

acelaya force-pushed the prettier-imports branch from 77fc596 to 621a707 Compare February 10, 2023 10:07

robertknight approved these changes Feb 10, 2023

View reviewed changes

acelaya added 2 commits February 13, 2023 17:26

Add prettier plugin to automatically order imports

110656b

Add stub package to temporarily cover non-optional prettier-plugin-so…

9ad9fe3

…rt-imports peer dependency

acelaya force-pushed the prettier-imports branch from 621a707 to 9ad9fe3 Compare February 13, 2023 16:27

acelaya merged commit bea79b3 into main Feb 13, 2023

acelaya deleted the prettier-imports branch February 13, 2023 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prettier plugin to automatically order imports #5237

Add prettier plugin to automatically order imports #5237

acelaya commented Feb 9, 2023 •

edited

Loading

acelaya Feb 9, 2023 •

edited

Loading

acelaya Feb 9, 2023

acelaya Feb 9, 2023

robertknight Feb 9, 2023

robertknight left a comment •

edited

Loading

acelaya commented Feb 9, 2023

acelaya Feb 9, 2023

robertknight Feb 9, 2023

acelaya commented Feb 9, 2023

robertknight left a comment

robertknight Feb 9, 2023

acelaya Feb 9, 2023

robertknight Feb 9, 2023

robertknight Feb 9, 2023

robertknight Feb 9, 2023

acelaya commented Feb 9, 2023

codecov bot commented Feb 9, 2023 •

edited

Loading

robertknight commented Feb 10, 2023 •

edited

Loading

acelaya Feb 10, 2023 •

edited

Loading

acelaya commented Feb 10, 2023

robertknight left a comment

robertknight Feb 10, 2023

robertknight Feb 10, 2023

robertknight Feb 10, 2023

acelaya commented Feb 10, 2023 •

edited

Loading

robertknight left a comment

Add prettier plugin to automatically order imports #5237

Add prettier plugin to automatically order imports #5237

Conversation

acelaya commented Feb 9, 2023 • edited Loading

acelaya Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment • edited Loading

Choose a reason for hiding this comment

acelaya commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Feb 9, 2023

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Feb 9, 2023

codecov bot commented Feb 9, 2023 • edited Loading

Codecov Report

robertknight commented Feb 10, 2023 • edited Loading

acelaya Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

acelaya commented Feb 10, 2023

robertknight left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acelaya commented Feb 10, 2023 • edited Loading

robertknight left a comment

Choose a reason for hiding this comment

acelaya commented Feb 9, 2023 •

edited

Loading

acelaya Feb 9, 2023 •

edited

Loading

robertknight left a comment •

edited

Loading

codecov bot commented Feb 9, 2023 •

edited

Loading

robertknight commented Feb 10, 2023 •

edited

Loading

acelaya Feb 10, 2023 •

edited

Loading

acelaya commented Feb 10, 2023 •

edited

Loading