-
Notifications
You must be signed in to change notification settings - Fork 1.5k
chore(imports): Reorganize boilerplate imports and adhere to import/order
linter rule
#2958
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
Conversation
…ng of imports Also updates all assets to use their typescript alias instead of a relative path to the asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR standardizes and reorganizes import statements across the boilerplate codebase by enforcing the import/order eslint rule and making import paths more consistent. Key changes include:
- Converting relative paths to use absolute aliases (e.g., "@/theme" for theme and "assets/…" for images/icons).
- Reordering and grouping import statements to comply with the new eslint configuration.
- Extracting configuration items for prettier, jest, and eslint into dedicated files and updating related documentation.
Reviewed Changes
Copilot reviewed 76 out of 76 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
boilerplate/app/components/Toggle/Toggle.tsx | Updated import paths for theme and text components to absolute aliases. |
boilerplate/app/components/Toggle/Switch.tsx | Reordered imports and applied absolute alias for theme-related imports. |
boilerplate/app/components/Toggle/Radio.tsx | Updated import paths to use "@/theme" and reordered grouped imports. |
boilerplate/app/components/Toggle/Checkbox.tsx | Converted relative theme import to "@/theme" and reorganized useAppTheme import. |
boilerplate/app/components/TextField.tsx | Updated import paths for theme and text to use absolute aliases. |
boilerplate/app/components/Text.tsx | Adjusted import order by moving React-specific imports to the top. |
boilerplate/app/components/Text.test.tsx | Refactored import statement ordering for consistency. |
boilerplate/app/components/Screen.tsx | Changed theme and safe area insets imports to use absolute paths. |
boilerplate/app/components/ListView.tsx | Reordered imports ensuring isRTL is imported from "@/i18n". |
boilerplate/app/components/ListItem.tsx | Updated import paths for theme, Icon, and Text components. |
boilerplate/app/components/Icon.tsx | Standardized asset require paths to use the new assets alias. |
boilerplate/app/components/Header.tsx | Reorganized and updated import paths for theme, safe area, Icon and Text. |
boilerplate/app/components/EmptyState.tsx | Updated asset path for images and reordered component imports. |
boilerplate/app/components/Card.tsx | Applied absolute alias for theme and reordered text imports. |
boilerplate/app/components/Button.tsx | Converted theme import to "@/theme" and reorganized text imports. |
boilerplate/app/app.tsx | Reordered key imports (initI18n, KeyboardProvider, useFonts) for consistency. |
boilerplate/README.md | Updated asset paths in documentation to reflect the new aliasing. |
boilerplate/.eslintrc.js | Adjusted error message and added import/order settings for consistency. |
.prettierrc | New file added with updated formatting rules. |
.eslintrc.js | New root ESLint configuration file with TypeScript settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going through this carefully! Looks good to me
@@ -1,6 +1,7 @@ | |||
export * from "./AutoImage" | |||
export * from "./Button" | |||
export * from "./Card" | |||
export * from "./EmptyState" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate thought from this PR but maybe we drop barrel files? I think most of us have moved away from that pattern on projects - what are your thoughts there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We moved away from barrel files on my current project and it reduced a lot of dependency cycles. Could be useful to check Ignite and see how many cycles are in there by default right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in essence we'd turn the imports from this:
import {
Button,
ButtonAccessoryProps,
Card,
EmptyState,
Icon,
ListView,
Screen,
Switch,
Text,
} from "@/components"
to this?
import { Button, ButtonAccessoryProps } from "@/components/Button"
import { Card } from "@/components/Card"
import { EmptyState } from "@/components/EmptyState"
import { Icon } from "@/components/Icon"
import { ListView } from "@/components/ListView"
import { Screen } from "@/components/Screen"
import { Switch } from "@/components/Toggle/Switch"
import { Text } from "@/components/Text"
Currently reading up on this and will post more thoughts after i run some tests and analysis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to using direct import from import aliases instead of barrel files reducing dependency cycles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea of managing a public exports boundary is better enforced using package.json#exports
between package modules rather than trying to enforce module boundaries within the same workspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we are using dependency-cruiser on a client project to report dependency cycles outside of metro as a part of linting and it works well. The benefit of this approach is that we can keep using barrel files if desired and it will warn when it creates a dependency cycle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah wherever the components are imported they'd end up on separate lines and the index.ts
in app/components
gets deleted. But it also exists in other directories:
There's also one in the models
, navigators
and screens
dir. Some export *
s in theme.
And then we'd have to update the ignite/templates
to not import from the barrel index as well.
Does that sound right, @coolsoftwaretyler ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshuayoes I used madge
to find the circular deps. I think adding a circular-deps test to the CI to make sure there are none is a good idea - #2959
I've tested this extensively and will merge with |
…all changes Folowup from #2958 after it was merged.
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
I noticed the ignite imports growing inconsistent over time sometimes using the typescript
@/*
import path and other times using relative paths. This PR is an attempt to solve that problem by implementing and enforcing theimport/order
eslint rule.This is a fairly large reorganization of the imports section of the boilerplate using the
import/order
eslint rule to separate and organize the import sections of the boilerplate codebase.prettier
,jest
, andeslint
configuration for the main ignite-cli out into their own files from inside the mainpackage.json
file. No changes to the actual configs*../assets/*
to use the typescript aliasassets/*
@/*
typescript alias. for instance,import { $styles } from "../theme"
becameimport { $styles } from "@/theme"
. This was NOT a find/replace-all, but a careful and methodical replacement throughout the boilerplate, leaving some spots with the relative import where it made sense (mostly in theapp.tsx
and in the demo screen files referencing each other).Checklist
README.md
and other relevant documentation has been updated with my changes