Resolve circular dependencies on JS side #2970

latekvo · 2024-06-28T12:52:27Z

This PR removes circular dependencies found via:

yarn madge --extensions js,ts,tsx --circular src

…ub.com/software-mansion/react-native-gesture-handler into @latekvo/resolve-circular-dependencies

m-bert

Looks good, let's hear what @j-piasecki has to say 😅

src/RNGestureHandlerModule.web.ts

src/components/GestureButtonsProps.tsx

src/components/GestureButtons.tsx

src/handlers/FlingGestureHandler.ts

src/components/touchables/GenericTouchableProps.tsx

src/web_hammer/NodeManager.ts

src/handlers/gestureCommonUtils.ts

src/handlers/handlerTag.ts

j-piasecki · 2024-07-02T08:16:57Z

@m-bert I think you mentioned adding a CI workflow dedicated to making sure we don't introduce another cycle by accident, is that happening?

m-bert · 2024-07-02T08:31:21Z

@j-piasecki Yes, I did mention adding CI workflow 😅 In order for it to make sense we have to get rid of all circular dependencies. At the beginning we didn't care that much about the one connected with Hammer, so it was postponed. Now that I see that we did remove all circular dependencies, I think adding CI workflow would be good idea.

Also, we could add it to pre-commit hook, let me know what do you think.

@latekvo could you add this workflow?

latekvo · 2024-07-02T08:35:28Z

@m-bert alright, should I add it to this PR, or do you think it deserves one of it's own?

m-bert · 2024-07-02T08:50:52Z

I think it would be nice to add it in this PR 😅

latekvo · 2024-07-02T10:36:46Z

New madge workflow

m-bert · 2024-07-02T10:44:01Z

.github/workflows/static-root-checks.yml

+      - name: Check for circular dependencies
+        run: yarn madge --extensions js,ts,tsx --circular src


Would it be possible to add script in our package.json and then use it here? Like yarn circular-dependency-check. This way it would be easier to run this in our repo (we wouldn't have to remember all those arguments and so on).

ok, will do

don't we want a script for both the JS and the native side in this case?
or is this a bit far fetched for now?

Is madge capable of doing such things?

Also keep in mind that in order to add this script we would have to have 0 circular dependencies on native side (I don't say we have them, that's something that we have to check).

So in conclusion, it would be great to have such script for native, but not in this PR.

Although madge likely won't work, for kotlin there is a tool called Classycle which could be made into a workflow.

But of course, i don't think doing this in this PR would be a good idea either 😄
I was just asking if it was something you'd like to be done.

I think it could be done, though first, it would be great to check how many of them are in the codebase to have an idea of the effort it would take.

m-bert · 2024-07-02T11:29:40Z

package.json

@@ -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"


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)

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.

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.

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).

#2973
@j-piasecki

latekvo added 11 commits June 28, 2024 13:54

resolve circular dependencies in gestureHandlerCommon

366ecc8

move event payloads to separate file to resolve circular dependencies

b179bb8

resolve button props circ deps

b5f7b2d

resolve generic and native touchable circ deps

736ddef

fix handler registry circ dep

188622d

fix invalid autolinter suggestion

b65d478

fix invalid autolinter suggestion

ce2c30a

Merge branch '@latekvo/resolve-circular-dependencies' of https://gith…

c2843e6

…ub.com/software-mansion/react-native-gesture-handler into @latekvo/resolve-circular-dependencies

fix invalid package imports

129258b

fix missed invalid import

ccd6e59

resolve last hammer circ dep

fafec8f

latekvo marked this pull request as ready for review July 1, 2024 13:06

latekvo requested review from m-bert and j-piasecki July 1, 2024 13:06

m-bert reviewed Jul 1, 2024

View reviewed changes

src/RNGestureHandlerModule.web.ts Outdated Show resolved Hide resolved

j-piasecki reviewed Jul 1, 2024

View reviewed changes

latekvo added 8 commits July 1, 2024 17:48

roll back node manager split

8ecf756

change gestureButtonProps from tsx to ts

2e3d271

optimize imports for prop files

58f8d3e

optimize imports for type importing

3221b96

optimize prop imports for type importing

35eee5c

rename file as per suggestion

71ecccc

rename one-function file as per suggestion

b3ed53a

rename and optimize file as per suggestion

df5e728

latekvo requested review from j-piasecki and m-bert July 2, 2024 07:48

add yarn madge workflow

045fd66

m-bert reviewed Jul 2, 2024

View reviewed changes

simplify workflow

52a970b

m-bert reviewed Jul 2, 2024

View reviewed changes

update function name as per request

c8cc982

j-piasecki approved these changes Jul 2, 2024

View reviewed changes

m-bert approved these changes Jul 2, 2024

View reviewed changes

latekvo merged commit 8290ad4 into main Jul 2, 2024
2 checks passed

latekvo deleted the @latekvo/resolve-circular-dependencies branch July 2, 2024 12:31

m-bert mentioned this pull request Jul 11, 2024

Require cycle with react native web with v2.16.1 #2989

Closed

2lambda123 mentioned this pull request Aug 21, 2024

[Snyk] Upgrade react-native-gesture-handler from 2.17.1 to 2.18.0 2lambda123/abp#162

Merged

Cabreira97 mentioned this pull request Aug 23, 2024

[Snyk] Upgrade react-native-gesture-handler from 2.16.2 to 2.18.1 Cabreira97/Connect-Tech#3

Open

hirenapatel1109 mentioned this pull request Aug 28, 2024

[4.0.0-alpha12] Metro error on Expo 51 / Web: Cannot access 'computedOffsetXValueWithAutoFillData' before initialization dohooo/react-native-reanimated-carousel#634

Open

ColeCoffman mentioned this pull request Aug 30, 2024

[Snyk] Upgrade react-native-gesture-handler from 2.1.3 to 2.18.1 ColeCoffman/JBApp#151

Open

Sachin-UI mentioned this pull request Sep 9, 2024

[Snyk] Upgrade: react, , , , , , , react-native-gesture-handler, react-native-reanimated, react-native-safe-area-context, react-native-screens, react-native-svg Sachin-UI/expensetrackerapp#4

Open

AKJUS mentioned this pull request Sep 12, 2024

[Snyk] Upgrade: , , , , lucide-react-native, react-native, react-native-gesture-handler, react-native-pager-view, react-native-reanimated, react-native-safe-area-context, react-native-screens, react-native-svg AKJUS/6pm#3

Open

ananichoumchoum mentioned this pull request Sep 13, 2024

[Snyk] Upgrade: react, , expo, expo-camera, react-native-gesture-handler, react-native-paper, react-native-reanimated, react-native-safe-area-context, react-native-screens ananichoumchoum/chatter-box#3

Open

AKJUS mentioned this pull request Sep 13, 2024

[Snyk] Upgrade: , , , , lucide-react-native, react-native, react-native-gesture-handler, react-native-pager-view, react-native-reanimated, react-native-safe-area-context, react-native-screens, react-native-svg AKJUS/6pm#4

Open

SanteriMertakorpi mentioned this pull request Sep 20, 2024

[Snyk] Upgrade: expo, expo-dev-client, expo-router, react-native-gesture-handler, react-native-paper, react-native-reanimated, react-native-safe-area-context, react-native-screens SanteriMertakorpi/Restaurant-menus#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve circular dependencies on JS side #2970

Resolve circular dependencies on JS side #2970

latekvo commented Jun 28, 2024

m-bert left a comment

j-piasecki commented Jul 2, 2024

m-bert commented Jul 2, 2024

latekvo commented Jul 2, 2024

m-bert commented Jul 2, 2024

latekvo commented Jul 2, 2024 •

edited

Loading

m-bert Jul 2, 2024

latekvo Jul 2, 2024

latekvo Jul 2, 2024

m-bert Jul 2, 2024

latekvo Jul 2, 2024

j-piasecki Jul 2, 2024

m-bert Jul 2, 2024

latekvo Jul 2, 2024

m-bert Jul 2, 2024

j-piasecki Jul 2, 2024

latekvo Jul 2, 2024

latekvo Jul 2, 2024

		- name: Check for circular dependencies
		run: yarn madge --extensions js,ts,tsx --circular src

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

Resolve circular dependencies on JS side #2970

Resolve circular dependencies on JS side #2970

Conversation

latekvo commented Jun 28, 2024

m-bert left a comment

Choose a reason for hiding this comment

j-piasecki commented Jul 2, 2024

m-bert commented Jul 2, 2024

latekvo commented Jul 2, 2024

m-bert commented Jul 2, 2024

latekvo commented Jul 2, 2024 • edited Loading

New madge workflow

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

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

latekvo commented Jul 2, 2024 •

edited

Loading