Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Group organized imports by distance #4725

Merged
merged 18 commits into from
Jul 31, 2023

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Jul 25, 2023

Summary

This PR groups imports using a fixed order. This is similar to what is proposed in #42 although the exact groups is slightly different. I have tried to follow the suggestion in that discussion that the "philosophy in general is that code closer to the file is imported after code further away" and attempted to implement this in a way that does not require further config changes.

I have not yet updated any documentation, I'll be happy to do so if you think this approach is worthwhile to pursue.

Test Plan

I've added a test case for import groups.

@netlify
Copy link

netlify bot commented Jul 25, 2023

Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 90da7f5
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/64c76b619f88bf00086f339d
😎 Deploy Preview https://deploy-preview-4725--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Langauge: JavaScript labels Jul 25, 2023
@ematipico
Copy link
Contributor

I don't really agree with the philosophy proposed in the discussion, which is really old btw, a lot of things have changed.

The reason why I don't agree with philosophy is because it doesn't take into consideration the possible side effects of external libraries.

If I had to suggest a change, I propose the exact opposite, where local imports are moved at the very end.

A real use case pops up in frontend applications, where CSS styles that belong to an external library must precede the local styles.

@arendjr
Copy link
Contributor Author

arendjr commented Jul 26, 2023

The discussion may be old, but the prior art they reference (https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md and https://github.com/trivago/prettier-plugin-sort-imports) are still current and widely used as far as I know.

I understand libraries may have side-effects, but that's the same reason the current organizeImports action is already considered unsafe, no?

If I had to suggest a change, I propose the exact opposite, where local imports are moved at the very end.

Maybe I'm misunderstanding, but that's what my proposal is also doing, no? It moves external imports to the top, and local imports to the bottom.

@github-actions github-actions bot added A-CLI Area: CLI L-CSS Language: CSS A-Parser Area: parser L-JSON Language: JSON labels Jul 28, 2023
@arendjr
Copy link
Contributor Author

arendjr commented Jul 28, 2023

FWIW, I've updated the tests, so hopefully they'll be all green now. I suspect @ematipico and I are actually mostly in agreement on what it should do, since this change would indeed cause "CSS styles that belong to an external" to be imported before local styles, so I haven't given up on it yet :)

Another thing I would like to mention is that I think this PR might be a useful step towards a lint rule such as "noMixedImportGroups" (name TBD) that could be used to mandate newlines between import groups. This would basically support the "newlines-between": "always" usecase found in eslint-plugin-import.

@github-actions github-actions bot added the A-Website Area: website and documentation label Jul 28, 2023
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

This is another fantastic contribution! Thank you!

@ematipico
Copy link
Contributor

Happy to merge once the conflicts is solved

@github-actions github-actions bot removed L-CSS Language: CSS A-Parser Area: parser labels Jul 31, 2023
@arendjr
Copy link
Contributor Author

arendjr commented Jul 31, 2023

Done!

@ematipico ematipico merged commit 204f7b5 into rome:main Jul 31, 2023
18 checks passed
@ematipico ematipico added PR-Needs documentation When a PR needs a follow up PR to udpate the documentation PR-Needs changelog line When a PR needs a follow up PR to udpate changelog labels Jul 31, 2023
@arendjr arendjr deleted the group-organized-imports-by-distance branch July 31, 2023 11:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-CLI Area: CLI A-Linter Area: linter A-Website Area: website and documentation L-JavaScript Langauge: JavaScript L-JSON Language: JSON PR-Needs changelog line When a PR needs a follow up PR to udpate changelog PR-Needs documentation When a PR needs a follow up PR to udpate the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants