🔨 feat: Migrate to `pnpm` #9516

nvh95 · 2022-12-04T15:01:56Z

Description

pnpm is faster and consumes less disk storage. Why don't use it? 😁

Related PR(s)

pnpm #9340

Features

Chores

Add build:watch script
Upgrade to vite@3 for app

codesandbox · 2022-12-04T15:01:57Z

Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

nvh95 · 2022-12-04T15:35:55Z

TODO:
To handle app folder when development, we have following options

Keep it as is, just link react and react-hook-form
Use pnpm workspace
Define "react-hook-form": "file:..", in app/package.json, then app folder will use react-hook-form at the parent directory. (However, vite might ignore the updates, need to configure vite)

Questions:

Why do we need to link react package as well? cc: @bluebill1049

nvh95 · 2022-12-04T16:06:11Z

@bluebill1049

Good news: Rollup and Test works! 🎉
In progress:
- start script
- compressed-size

bluebill1049 · 2022-12-04T21:46:32Z

Why do we need to link react package as well? cc: @bluebill1049

It was due to multiple react version issue, maybe it's no longer an issue.

bluebill1049 · 2022-12-04T21:46:59Z

@bluebill1049

Good news: Rollup and Test works! 🎉

In progress:

start script

compressed-size

That's awesome!

bluebill1049 · 2022-12-04T21:53:18Z

I think it's no longer an issue with react version:

the following works

yarn build:esm && yarn --cwd ./app link react react-hook-form && yarn --cwd ./app run dev

I think this is the best option: (It's fine without the updates too)

Define "react-hook-form": "file:..", in app/package.json, then app folder will use react-hook-form at the parent directory. (However, vite might ignore the updates, need to configure vite)

(issues with 2 react(s) in the past)

nvh95 · 2022-12-05T00:35:59Z

Not sure why there are some cypress tests fail.
cc@bluebill1049

nvh95 · 2022-12-05T00:38:32Z

FYI: just added build:watch for rollup to rebuild when file changes

bluebill1049 · 2022-12-05T00:44:55Z

thanks @nvh95 I will fix those cypress tests.

nvh95 · 2022-12-05T00:47:00Z

@bluebill1049 The cypress tests just passed after I rebased on master. I am not sure if your latest commit fixes it, or they are unstable tests. I will keep my eyes on them as well.
Thank you, Bill.

bluebill1049 · 2022-12-05T00:54:36Z

I see, that's awesome! Thanks a lot @nvh95

nvh95 · 2022-12-05T01:26:37Z

@bluebill1049 If by any chance when you got some time, can you help to look at this failed action?
I guess the issue is with the use of pnpm and yarn when we try to do the comparison.
Thanks.

https://github.com/react-hook-form/react-hook-form/actions/runs/3615848030/jobs/6093262592

bluebill1049 · 2022-12-05T01:37:53Z

@nvh95 sure will do.

bluebill1049 · 2022-12-05T01:43:53Z

looks like it's determined by the.lock file: https://github.com/preactjs/compressed-size-action/blob/master/src/index.js#L59-L67

bluebill1049 · 2022-12-05T01:47:24Z

on it's pulling the master, let's merge it in. It should just work.

it's try to pull the master to compare the size difference, hence it's not going to work out (yarn vs pnpm)

nvh95 · 2022-12-05T01:49:27Z

@bluebill1049 I'm checking all the boxes in the PR description. I will let you know when it's done and we are good to merge

bluebill1049 · 2022-12-05T01:50:39Z

@bluebill1049 I'm checking all the boxes in the PR description. I will let you know when it's done and we are good to merge

Awesome! thanks so much for this @nvh95 🙏 🥳

nvh95 · 2022-12-05T01:54:09Z

app/package.json

@@ -2,7 +2,7 @@
  "name": "react-hook-form",
  "version": "0.0.0",
  "scripts": {
-    "dev": "vite",
+    "dev": "vite --force",


@bluebill1049 --force is to invalidate the pre-bundle dependencies. This is a workaround. When you make changes to RHF, please restart the dev server of app.
We will try to improve this behavior to have real HMR. (See my comment in app/vite.config.ts for more)

Yea, no worries on that.

nvh95 · 2022-12-05T01:56:29Z

.github/workflows/api-extrator.yml

+        # Do we want to run with multiple versions of node?
+        node-version: [16]


@bluebill1049 It is recommended to specify which node version we use. FYI

sounds good 👍

nvh95 · 2022-12-05T01:57:25Z

.github/workflows/build-test.yml

+    strategy:
+      matrix:
+        # Do we want to run with multiple versions of node?
+        node-version: [16]


@bluebill1049 It is recommended to specify which node version we use. Also, we can test RHF on multiple node versions as well. For now, I just fixed it to node 16, we can change to following in the future if it's needed

Suggested change

strategy:

matrix:

# Do we want to run with multiple versions of node?

node-version: [16]

strategy:

matrix:

# Do we want to run with multiple versions of node?

node-version: [16, 18]

nvh95 · 2022-12-05T01:58:48Z

.github/workflows/build-test.yml

      - name: Test
        run: |
-          yarn test --ci
-          yarn test:type
+          pnpm run test --ci


pnpm test --ci fails,
pnpm run test --ci passed
Not have much time to check more details.
cc: @bluebill1049

haha that's strange, yea pnpm run test --ci is fine.

nvh95 · 2022-12-05T02:02:36Z

@bluebill1049
I tried to check all the scripts. These scripts fail, I think I need your help:

coverage
test:native

For script e2e, GitHub Actions seems fine. But when I run it locally, this test suite fails (setValueAsyncStrictMode.cy.ts ). Please help to double check.

Otherwise, my PR is ready to be merged 🥳. Please help to do some testing from your side.

Thank you very much for this awesome library.

bluebill1049 · 2022-12-05T02:30:48Z

I will take over and resolve the rest 🌷 thanks @nvh95 a lot 👍

- coverage - test:native - test:server

bluebill1049

🚀 This is really awesome and thanks very much for the huge effort!

sadeghbarati · 2022-12-05T06:47:44Z

app/package.json

@@ -13,7 +13,7 @@
    "joi": "^17.5.0",
    "react": "^17.0.1",
    "react-dom": "^17.0.1",
-    "react-hook-form": "^7.25.0",
+    "react-hook-form": "file:..",


@nvh95 Hi, I have question why you don't use workspace:* keyword here?

aha still no pnpm-workspace.yaml file on project root,

Hi @sadeghbarati. I considered that option #9516 (comment)

However, now, I am looking for the simplest solution, since this PR focuses on migrating to pnpm. In the next PR, I will adopt pnpm Workspace and use workspace:*.

Thanks for your suggestion 🤗.

Just saw your Tweet, Thanks for your efforts

bluebill1049 force-pushed the pnpm branch from 281aaaa to b2d9134 Compare December 4, 2022 23:17

nvh95 added 8 commits December 5, 2022 07:27

🔨 feat: replacing yarn by pnpm

d447a36

🔨 chore: Update package.json

e6ab840

🛠️ feat: Update GitHub Actions

147254c

🔨 feat: Install pnpm for compressed-size workflow

b4eaab5

🔨 feat: Use setup-node for compress-size.yml

3833901

fix: fix syntax

9f620f4

🔨 feat: use pnpm link for start command

1030127

🧪 feat: use pnpm for automation workflow

866f04d

nvh95 force-pushed the pnpm branch from b2d9134 to 866f04d Compare December 5, 2022 00:32

nvh95 added 2 commits December 5, 2022 07:32

feat: no need to link react

4d741c5

(issues with 2 react(s) in the past)

chore: add a build:watch script

6034507

feat: do not need to link rhf to app

a68cdfc

bluebill1049 marked this pull request as ready for review December 5, 2022 01:47

nvh95 commented Dec 5, 2022

View reviewed changes

remove the following script due to overlap

f83aa9a

- coverage - test:native - test:server

bluebill1049 approved these changes Dec 5, 2022

View reviewed changes

bluebill1049 merged commit c7210f1 into react-hook-form:master Dec 5, 2022

nvh95 deleted the pnpm branch December 5, 2022 04:04

sadeghbarati reviewed Dec 5, 2022

View reviewed changes

Mini-ghost mentioned this pull request Dec 17, 2022

replace all yarn in CONTRIBUTING.md with pnpm #9605

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🔨 feat: Migrate to `pnpm` #9516

🔨 feat: Migrate to `pnpm` #9516

nvh95 commented Dec 4, 2022 •

edited by bluebill1049

codesandbox bot commented Dec 4, 2022

nvh95 commented Dec 4, 2022 •

edited

nvh95 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022 •

edited

nvh95 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022 •

edited

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

nvh95 Dec 5, 2022

bluebill1049 Dec 5, 2022

nvh95 Dec 5, 2022

bluebill1049 Dec 5, 2022

nvh95 Dec 5, 2022

bluebill1049 Dec 5, 2022

nvh95 Dec 5, 2022

bluebill1049 Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 left a comment

sadeghbarati Dec 5, 2022

sadeghbarati Dec 5, 2022

nvh95 Dec 5, 2022

sadeghbarati Dec 5, 2022

		# Do we want to run with multiple versions of node?
		node-version: [16]

🔨 feat: Migrate to pnpm #9516

🔨 feat: Migrate to pnpm #9516

Conversation

nvh95 commented Dec 4, 2022 • edited by bluebill1049

Description

Related PR(s)

Features

Chores

codesandbox bot commented Dec 4, 2022

nvh95 commented Dec 4, 2022 • edited

nvh95 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022

bluebill1049 commented Dec 4, 2022 • edited

nvh95 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022 • edited

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

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

nvh95 commented Dec 5, 2022

bluebill1049 commented Dec 5, 2022

bluebill1049 left a 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

🔨 feat: Migrate to `pnpm` #9516

🔨 feat: Migrate to `pnpm` #9516

nvh95 commented Dec 4, 2022 •

edited by bluebill1049

nvh95 commented Dec 4, 2022 •

edited

bluebill1049 commented Dec 4, 2022 •

edited

bluebill1049 commented Dec 5, 2022 •

edited