2966: Migrating to Modern Yarn #2968

bahaaTuffaha · 2024-10-22T10:55:42Z

Short description

Upgraded to [email protected]

Proposed changes

yarnrc.yml will have nodeLinker: node-modules meaning its not depending on pnp more on that at (https://yarnpkg.com/features/pnp)
Note we can do hybrid pnp in the future look at https://yarnpkg.com/getting-started/recipes#hybrid-pnp--node_modules-mono-repo
nohoist is deprecated so I removed it and added nmHoistingLimits: workspaces
This yarn version at some places it complains about --silent so I removed some of them. Still looking for a way to silent these warnings.

Note: not tested on iOS and focused mainly on integrate native and integrate web app.

Side effects

Not sure

Testing

I used this time: Node 22.9.0 you can install by nvm
before yarn install make sure to run corepack enable once to activate Corepack.
I suggest to delete all node modules files from the repo then do yarn install.

Resolved issues

Fixes: #2966

LeandraH · 2024-10-22T11:15:30Z

Thank you, I probably won't manage to have time today to take a look at it but maybe also check out this old PR that was never merged to see what went wrong there and if this PR fixes the issues: #2588

bahaaTuffaha · 2024-10-22T11:25:09Z

Thank you, I probably won't manage to have time today to take a look at it but maybe also check out this old PR that was never merged to see what went wrong there and if this PR fixes the issues: #2588

I found out it needs to be bit more in the oven.. sorry.. yarn test is not working I'm investigating...I will convert this PR to Draft for now

LeandraH

Just a few small comments, as discussed on Mattermost, next thing to do is get the commands in the package.json files to work in their respective folders.

LeandraH · 2024-10-28T15:11:53Z

native/package.json

-    "ts:check": "tsc --build",
-    "prettier:check": "yarn prettier --ignore-path  ../.prettierignore --check .",
-    "prettier:write": "yarn prettier --ignore-path  ../.prettierignore --write .",
+    "ts:check": "yarn ts:check",


❓ That looks like a pretty circular reference to me?

https://yarnpkg.com/getting-started/qa#how-to-share-scripts-between-workspaces

Hmmm, sorry, but I don't understand what sharing scripts between different workspaces has to do with it being a circular reference. This command would run in the folder it is run in, in this case /native, not in the root. Is this maybe the wrong link? Or if not, could you elaborate a little on why you think this applies here?

Also, I thought we agreed that we want the commands that are run in a folder to only run in that folder? I guess we could add global commands to each package.json but I'm not sure that's necessary, you can always just do cd .. and then run the command globally there.

LeandraH · 2024-10-28T15:12:26Z

native/android/app/buildConfigs.gradle

@@ -39,7 +39,7 @@ def determineBuildConfigName() {
 }

 def createYarnProcess(buildConfigName, platform, command) {
-    return execCommand("yarn workspace --silent build-configs --silent manage $command $buildConfigName $platform")
+    return execCommand("yarn workspace build-configs manage $command $buildConfigName $platform")


❓ Why did you remove the silent flags?

Yarn complains about them and I didn't find any mention of it in the docs.
And I tried this also:
yarnpkg/yarn#788 (comment)

Interesting, looks like it has been completely removed, thanks for looking into it!
yarnpkg/berry#3542

LeandraH · 2024-10-28T15:22:08Z

native/package.json

@@ -31,16 +31,15 @@
    "android:reverse": "adb reverse tcp:8081 tcp:8081",
    "android:reinit": "adb shell pm clear com.integreat && adb shell am start com.integreat/com.integreat.MainActivity",
    "android:start-avd": "bash -c '$ANDROID_HOME/emulator/emulator -avd $($ANDROID_HOME/emulator/emulator -list-avds | head -n 1)'",
-    "test": "jest",
+    "test": "yarn g:test",


❌ Those are not the same thing, we want to be able to run only the native tests while in the native folder.

LeandraH · 2024-10-28T15:22:39Z

native/package.json

    "test:changed": "yarn run test -o",
    "test:e2e": "yarn workspace e2e test:native:browserstack",
-    "lint": "yarn stylelint && eslint --cache --cache-location ../.eslintcache .",
-    "stylelint": "stylelint --config stylelint.config.js \"src/**/*.{ts,tsx,css}\"",
+    "lint": "yarn g:lint",


❌ Same here

LeandraH · 2024-10-28T15:23:07Z

native/package.json

+    "prettier:check": "yarn g:prettier:check",
+    "prettier:write": "yarn g:prettier:write",


❌ And here

LeandraH · 2024-10-28T15:33:14Z

package.json

+    "lint": "yarn g:lint",
+    "g:lint": "eslint --cache --cache-location .eslintcache . && yarn workspace web stylelint && yarn workspace native stylelint",


❓ Why did you decide on those two different commands that do the same thing?

😅 one for the current dir and the other for other workspaces...

Two things:

This is at root, so there isn't really any code in this directory to be linted.

If I now execute yarn lint in the root directory, yarn checks in package.json what to do for the command lint and sees that it should execute yarn g:lint. Which is the command in line 18. So both commands do the same thing with line 17 just being a reference to line 18. I don't think that's necessary.

LeandraH · 2024-10-28T15:34:42Z

shared/package.json

@@ -24,6 +21,7 @@
    "minisearch": "^7.1.0",
    "normalize-path": "^3.0.0",
    "normalize-strings": "^1.1.1",
+    "react": "workspaces:web",


❓ Why did you decide to not keep that as a peer dependency?

LeandraH · 2024-10-28T16:53:44Z

web/package.json

@@ -36,6 +35,7 @@
    "build-configs": "0.0.1",
    "dompurify": "^3.1.6",
    "focus-trap-react": "^10.2.3",
+    "htmlparser2": "^9.1.0",


❓ Why did you add this?

LeandraH · 2024-10-28T16:53:54Z

web/package.json

@@ -87,6 +87,7 @@
    "raf": "^3.4.1",
    "react-refresh": "^0.14.2",
    "react-refresh-typescript": "^2.0.9",
+    "rrule": "2.8.1",


❓ And this?

…lassic-to-modern

2966: Updating to Modern Yarn

da2327f

bahaaTuffaha requested review from steffenkleinle, ztefanie, f1sh1918 and LeandraH as code owners October 22, 2024 10:55

bahaaTuffaha marked this pull request as draft October 22, 2024 11:25

bahaaTuffaha added 3 commits October 23, 2024 11:57

2966: still problem with web needs entities pack from shared

e5d241c

2966: Fixing scripts and make remove unnecessary adjustments

8932dfe

2966: Fixing scripts part 2

cc7af38

LeandraH reviewed Oct 28, 2024

View reviewed changes

bahaaTuffaha added 2 commits November 15, 2024 13:22

Merge remote-tracking branch 'origin' into 2966-Migrating-yarn-from-c…

352bd3c

…lassic-to-modern

2966: Changes part1

3acce30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

2966: Migrating to Modern Yarn #2968

2966: Migrating to Modern Yarn #2968

bahaaTuffaha commented Oct 22, 2024

LeandraH commented Oct 22, 2024

bahaaTuffaha commented Oct 22, 2024

LeandraH left a comment

LeandraH Oct 28, 2024 •

edited

Loading

bahaaTuffaha Oct 29, 2024

LeandraH Oct 29, 2024 •

edited

Loading

LeandraH Oct 28, 2024

bahaaTuffaha Oct 29, 2024

LeandraH Oct 29, 2024

LeandraH Oct 28, 2024

LeandraH Oct 28, 2024

LeandraH Oct 28, 2024

LeandraH Oct 28, 2024

bahaaTuffaha Oct 29, 2024

LeandraH Oct 29, 2024

LeandraH Oct 28, 2024

LeandraH Oct 28, 2024

LeandraH Oct 28, 2024

		"prettier:check": "yarn g:prettier:check",
		"prettier:write": "yarn g:prettier:write",

		"lint": "yarn g:lint",
		"g:lint": "eslint --cache --cache-location .eslintcache . && yarn workspace web stylelint && yarn workspace native stylelint",

2966: Migrating to Modern Yarn #2968

Are you sure you want to change the base?

2966: Migrating to Modern Yarn #2968

Conversation

bahaaTuffaha commented Oct 22, 2024

Short description

Proposed changes

Side effects

Testing

Resolved issues

LeandraH commented Oct 22, 2024

bahaaTuffaha commented Oct 22, 2024

LeandraH left a comment

Choose a reason for hiding this comment

LeandraH Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeandraH Oct 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

LeandraH Oct 28, 2024 •

edited

Loading

LeandraH Oct 29, 2024 •

edited

Loading