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.

Sign up for GitHub

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

Jump to bottom

Resolve circular dependencies on JS side #2970

Merged
merged 22 commits into from
Jul 2, 2024
Merged

Resolve circular dependencies on JS side #2970

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
366ecc8
resolve circular dependencies in gestureHandlerCommon
latekvo Jun 28, 2024
b179bb8
move event payloads to separate file to resolve circular dependencies
latekvo Jun 28, 2024
b5f7b2d
resolve button props circ deps
latekvo Jul 1, 2024
736ddef
resolve generic and native touchable circ deps
latekvo Jul 1, 2024
188622d
fix handler registry circ dep
latekvo Jul 1, 2024
b65d478
fix invalid autolinter suggestion
latekvo Jul 1, 2024
ce2c30a
fix invalid autolinter suggestion
latekvo Jul 1, 2024
c2843e6
Merge branch '@latekvo/resolve-circular-dependencies' of https://gith…
latekvo Jul 1, 2024
129258b
fix invalid package imports
latekvo Jul 1, 2024
ccd6e59
fix missed invalid import
latekvo Jul 1, 2024
fafec8f
resolve last hammer circ dep
latekvo Jul 1, 2024
8ecf756
roll back node manager split
latekvo Jul 1, 2024
2e3d271
change gestureButtonProps from tsx to ts
latekvo Jul 2, 2024
58f8d3e
optimize imports for prop files
latekvo Jul 2, 2024
3221b96
optimize imports for type importing
latekvo Jul 2, 2024
35eee5c
optimize prop imports for type importing
latekvo Jul 2, 2024
71ecccc
rename file as per suggestion
latekvo Jul 2, 2024
b3ed53a
rename one-function file as per suggestion
latekvo Jul 2, 2024
df5e728
rename and optimize file as per suggestion
latekvo Jul 2, 2024
045fd66
add yarn madge workflow
latekvo Jul 2, 2024
52a970b
simplify workflow
latekvo Jul 2, 2024
c8cc982
update function name as per request
latekvo Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/static-root-checks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,4 @@ jobs:
- name: Lint
run: yarn lint:js-root
- name: Check for circular dependencies
run: yarn madge --extensions js,ts,tsx --circular src
run: yarn checkForCircularDependencies
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"lint:js": "eslint --ext '.js,.ts,.tsx' src/ example/src FabricExample/src MacOSExample/src && yarn prettier --check './{src,example,FabricExample,MacOSExample}/**/*.{js,jsx,ts,tsx}'",
"lint:js-root": "eslint --ext '.js,.ts,.tsx' src/ && yarn prettier --check './src/**/*.{js,jsx,ts,tsx}'",
"lint:android": "./android/gradlew -p android spotlessCheck -q",
"checkIntegrity": "(cd ./FabricExample/android && ./gradlew generateCodegenArtifactsFromSchema -PskipCodegenCopyTask) && (cd ./android && ./gradlew checkIntegrityBetweenArchitectures)"
"checkIntegrity": "(cd ./FabricExample/android && ./gradlew generateCodegenArtifactsFromSchema -PskipCodegenCopyTask) && (cd ./android && ./gradlew checkIntegrityBetweenArchitectures)",
"checkForCircularDependencies": "yarn madge --extensions js,ts,tsx --circular src"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"checkForCircularDependencies": "yarn madge --extensions js,ts,tsx --circular src"
"circularDependencyCheck": "yarn madge --extensions js,ts,tsx --circular src"

I think it is enough, we don't have to overcomplicate that name. Also I think using - as separator is more readable than camelCase, but since we have checkIntegrity script we can leave that (or we can change both, cc @j-piasecki)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, i agree but preferred to follow the convention instead of having both cases in a single filename next to eachother.

Same for the name, but since you prefer the latter one, I'll change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I get what you mean by "same for the name" 😅 Currently we have only one script that starts with check so I think it is a bit different compared to - and camelCase. Starting with check prefix isn't that bad, but having many scripts starting with the same prefix would make tab autocomplete a bit frustrating 😅

This is a topic for discussion and I think that following Reanimated convention would be better, but let's hear @j-piasecki opinion on that.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can merge it as is, but create a follow-up that will take care of this, given that we already have a ts-check script (to use dashes instead of camelCase).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#2973
@j-piasecki

},
"react-native": "src/index.ts",
"main": "lib/commonjs/index.js",
Expand Down
Loading