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

Add prettier plugin to sort imports in a consistent and automated way #877

Closed
wants to merge 3 commits into from

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Feb 27, 2023

This adds @trivago/prettier-plugin-sort-imports to the project, so that we can ensure a consistent and automated imports sorting on front-end components via prettier.

This was already introduced in client. See hypothesis/client#5237 and hypothesis/client#5279

@acelaya acelaya requested a review from robertknight February 27, 2023 08:54
"singleQuote": true
"singleQuote": true,
"importOrder": [
"^.\\/components\\/patterns\\/(.*)Components$",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regular expression ensures the imports for legacy components in the pattern library router are placed in their own group (as they are now), to ease eventual removal.

Once those are removed we can also remove this regexp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding removal of this to the list of small tasks for version 6: #876

@robertknight
Copy link
Member

robertknight commented Feb 27, 2023

I suggest getting @lyzadanger to look at this, since she has been doing most of the work in this repo. I confirm that the changes are consistent with the import ordering changes in the client and lms repos.

import PanelComponents from './components/patterns/PanelComponents';
import SpinnerComponents from './components/patterns/SpinnerComponents';
import TableComponents from './components/patterns/TableComponents';
import ThumbnailComponents from './components/patterns/ThumbnailComponents';
Copy link
Contributor Author

@acelaya acelaya Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The block of legacy page imports used to be at the bottom. However, I couldn't find an easy way to keep that behavior. The closest I found was a way to keep them as their own group, but at the top instead of the bottom (see this comment).

Since we are soon removing this, I thought this would be good enough for now, but if it is preferred to keep the previous behavior, I can check a bit further and try to find a better regular expression.

For context, the reason it is at the top is because this plugin matches imports against regular expressions defined in importOrder prettier config. Each regexp defined there becomes an import group.

The one we usually use (^[./]) is too broad, and matches legacy pages as well, resulting in a mixture of imports.

I tried adding ^.\/components\/patterns\/(.*)Components$ after, which only matches legacy pages, but for the reason explained above, it does not work unless we put it before ^[./].

I have tried using a regexp which is "the opposite" of ^.\/components\/patterns\/(.*)Components$, but it's tricky and has weird side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyzadanger your feedback here would be very valuable.

@codecov
Copy link

codecov bot commented Feb 27, 2023

Codecov Report

Merging #877 (1b09530) into main (79f8379) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #877   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           59        59           
  Lines          710       710           
  Branches       255       255           
=========================================
  Hits           710       710           
Impacted Files Coverage Δ
src/components/Checkbox.js 100.00% <ø> (ø)
src/components/Dialog.js 100.00% <ø> (ø)
src/components/Panel.js 100.00% <ø> (ø)
src/components/data/DataTable.tsx 100.00% <ø> (ø)
src/components/data/Scroll.js 100.00% <ø> (ø)
src/components/data/Table.js 100.00% <ø> (ø)
src/components/data/TableBody.js 100.00% <ø> (ø)
src/components/data/TableCell.tsx 100.00% <ø> (ø)
src/components/data/TableFoot.tsx 100.00% <ø> (ø)
src/components/data/TableHead.js 100.00% <ø> (ø)
... and 16 more

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

@acelaya acelaya requested review from lyzadanger and removed request for robertknight February 27, 2023 10:11
Copy link
Contributor

@lyzadanger lyzadanger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this bad boy merged!

"singleQuote": true
"singleQuote": true,
"importOrder": [
"^.\\/components\\/patterns\\/(.*)Components$",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding removal of this to the list of small tasks for version 6: #876

@acelaya
Copy link
Contributor Author

acelaya commented Feb 27, 2023

Something went horribly wrong while resolving conflicts in yarn.lock, so I'm just replacing this PR with #885

@acelaya acelaya closed this Feb 27, 2023
@lyzadanger lyzadanger deleted the sort-imports branch March 1, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants