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.

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

Upgrade to pnpm #444

Closed
wants to merge 25 commits into from
Closed

Upgrade to pnpm #444

wants to merge 25 commits into from

Conversation

technophile-04
Copy link
Collaborator

@technophile-04 technophile-04 commented Jul 24, 2023

Description

Updated the pkgManager to pnpm for trial as discussed in #419

For deploying contract you need to use pnpm hardhat:deploy instead of just pnpm deploy because deploy seem reserved keyword
Will try to find if there exist any solution to above problem 🙌....But I think its ready to test and give a trial

This is solved please check #444 (comment) 🙌

@technophile-04 technophile-04 marked this pull request as draft July 24, 2023 17:33
@rin-st
Copy link
Member

rin-st commented Jul 25, 2023

I tried base scripts chain, hardhat:deploy, start, vercel and some others and everything works as expected 👍 . pnpm hardhat:deploy is a little bit longer than it was, but it becomes more clear what is deployed, at least for newbies.

package.json Outdated Show resolved Hide resolved
@rin-st
Copy link
Member

rin-st commented Jul 25, 2023

and it shows me error when I use ctrl + c in pnpm chain terminal.
image

As I understand it doesn't affect anything. Fast googling didn't help me but we need to try to get rid of it.

package.json Outdated Show resolved Hide resolved
pnpm-workspace.yaml Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@carletex
Copy link
Member

Great job @technophile-04! Thank you for putting this together.

Tested it and everything works great.

For deploying contract you need to use pnpm hardhat:deploy instead of just pnpm deploy because deploy seem reserved keyword

Damn, that's going to be a hard pill to swallow for some people haha. The deploy command is the most used one + we are switching to pnpm: yarn deploy => pnpm hardhat:deploy is going to be hard to sell haha. As Rinat said, probably for newcomers it doesn't matter. We could also do contract:deploy.

@Pabl0cks
Copy link
Collaborator

It's working great on Windows, GJ @technophile-04 !!

@technophile-04
Copy link
Collaborator Author

As I understand it doesn't affect anything. Fast googling didn't help me but we need to try to get rid of it.

pnpm is less friendly than yarn when you write a non existing command:

I have been playing around with some configs mentioned here -> https://pnpm.io/npmrc#cli-settings but nothing works :(


I have also noticed a bug i.e patch is not applied, because if you deploy two contracts you get :

YourContarct is selected but not highlight hydration warning in console
Screenshot 2023-07-28 at 6 58 53 PM Screenshot 2023-07-28 at 6 58 31 PM

This signifies that patch is not working checkout #453 (comment) for more description

Having a look at it 🙌

@technophile-04
Copy link
Collaborator Author

Update I got it working in pnpm-pathc branch, but to get it working I need to do some manual work :(, and also there if you run any pnpm command you always get this warning :

The field "pnpm" was found in /Users/..../scaffold-eth-2/packages/next js/package.json. This will not take effect. You should configure "pnpm" at the root of the workspace instead.

I have created an issue on pnpm -> pnpm/pnpm#6885

Let's see what they have to say if there is some direct way to do this otherwise we merge pnpm-pathc into this branch

I will keep tinkering around it 🙌

@technophile-04
Copy link
Collaborator Author

fine for now, except pnpm hardhat:test. It's throwing this exception:

On my windows desktop it takes 10s to pnpm install on subsequent installs, vs 2.5s from yarn install.

This is strange :(, @Pabl0cks could you please just check your pnpm version and making sure its above >= 8.7.1 because I had this similar 4:1 ratio before version 8.7.1 because of pnpm/pnpm#6890 but its fixed now

Also regarding pnpm hardhat:test could you just try using pnpm run hardhat:test and also share an SS of full output 🙌


I agree with you guys !

I only vote for pnpm because it will make installation via CLI a lot faster and consistent, as we will already be having "pnpm-lock.yam" inside each extension / package right ? (LOL the main motive for me to create this PR was this reason) but if we are not able to include pnpm-lock.yaml inside each package then I don't think its worth it (Please correct me If I missed something or lol I am wrong path or missed some other PRO's)

Regarding #419 this could be solved without using pnpm by uploading the whole Monorepo to vercel (which in turn will contain yarn.lock) but people need to mention "./packages/nextjs" when they run "yarn vercel" as mentioned in #231 (comment) which might be ok then typing "pnpm run" every time

We should probably have everything prepared and push the red button 🔴 when ready.

Lol yup, I was wondering whether we should create another PR parallel / test locally at CLI branch utilizing pnpm to install dependency and see if everything works nicely there and then we can merge this PR once we are sure ?

Tysm guys!!

@Pabl0cks
Copy link
Collaborator

Pabl0cks commented Sep 8, 2023

This is strange :(, @Pabl0cks could you please just check your pnpm version and making sure its above >= 8.7.1 because I had this similar 4:1 ratio before version 8.7.1 because of pnpm/pnpm#6890 but its fixed now

Also regarding pnpm hardhat:test could you just try using pnpm run hardhat:test and also share an SS of full output 🙌

I had 8.6.10. Updated to 8.7.4 and installation went down to 8.6 seconds 🙌.

I still get the REPORT_GAS=true error, and I've noticed a error during the install, don't know if it can affect my pnpm hardhat:test results. Here is the SS with the 3 commands:

image

Here the SS using PowerShell instead:

image

@technophile-04 technophile-04 linked an issue Sep 12, 2023 that may be closed by this pull request
@carletex
Copy link
Member

carletex commented Sep 14, 2023

@Pabl0cks I think Windows doesn't recognize env vars like that on the package.json.

Can you check if you get the same error when running yarn hardhat:test on main? We might need to use something like https://www.npmjs.com/package/cross-env


Any thoughts on this? #444 (comment)

@technophile-04
Copy link
Collaborator Author

It's a bit weird to use pnpm <script> for all the scripts except for deploy. Should we recommend (in the docs) to use pnpm run for all the scripts, or do you think is it good as it is?

I would vote for using pnpm run <script> just to keep things consistent and also pnpm suggests it to run that way.

Also noticed pnpm run gives a slightly correct ERR_MESAAGE output if you misspell :
Screenshot 2023-09-14 at 8 04 23 PM

Lol but yeah I am kind of 51% and 49% so anything works 🙌

@Pabl0cks
Copy link
Collaborator

Can you check if you get the same error when running yarn hardhat:test on main?

Just tried a fresh scaffold-eth-2 and yarn hardhat:test works fine on main. Here is the sshot:

image

@rin-st
Copy link
Member

rin-st commented Sep 24, 2023

@technophile-04 since you fixed wagmi/viem types, original problem disappeared.

The only problem with yarn for me is this
image

But if we move that simple type to our project everything works again with yarn without locking wagmi/viem versions.

So it seems we don't need lockfiles anymore. Am I missing something?

ps. sorry for this comment @technophile-04 😄

@technophile-04
Copy link
Collaborator Author

The only problem with yarn for me is this image

But if we move that simple type to our project everything works again with yarn without locking wagmi/viem versions.

Tysm Rinat, just created PR at #541

So it seems we don't need lockfiles anymore. Am I missing something?

Do you, mean to say separate lockfiles?

I think although the viem/wagmi issue is solved but it might with other libraries that use installs when using SE-2 for example checkout => scaffold-eth/se-2-challenges#91 (this can solved as mentioned in #444 (comment) but it decreases DX while deploying to vercel )

also checkout Carlos comment here => #290 (comment) 🙌

@rin-st
Copy link
Member

rin-st commented Sep 24, 2023

Tysm Rinat, just created PR at #541

👍 ✅

Do you, mean to say separate lockfiles?

Yep

I think although the viem/wagmi issue is solved but it might with other libraries that use installs when using SE-2 for example checkout => scaffold-eth/se-2-challenges#91

hm, agree, you're right. And I'm glad you're right )

(this can solved as mentioned in #444 (comment) but it decreases DX while deploying to vercel )

I tried it, doesn't work for me. It asks for additional packages like node-gyp. Didn't try to install it, I think it's not a good solution

❤️

@technophile-04
Copy link
Collaborator Author

Can confirm that the error Pablo got #444 (comment) here is with all Windows users, tried it on my friend's Windows machine and got the same error.

After doing some digging I got it working on Windows by first running (needs to be run only once in life) :

pnpm config set shell-emulator=true

after doing this pnpm run hardhat:test worked fine (@Pabl0cks could you please try this once ? )

Reasoning :

checkout description for shell-emulator as pnpm says :

When true, pnpm will use a JavaScript implementation of a bash-like shell to execute scripts

Also, I think the reason why it works with yarn by default is because yarn handles it already internally check out this => yarn/features/scripting/portable-shell


Just some thoughts while I was playing on Windows:

Lol don't know why but I don't feel confident merging this PR (at least because of windows)

  • For some reason it took lot of time while doing install almost around 5m 6.6s (It might be due network condition but yarn install was comparatively a lot better)
  • On subsequest install was getting the same cpu-feature error as Pablo showed in his first image in Upgrade to pnpm #444 (comment) (planning to debug this next 🙌)

@rin-st
Copy link
Member

rin-st commented Sep 25, 2023

After doing some digging I got it working on Windows by first running (needs to be run only once in life)

Since your pnpm config is inside .npmrc file you can just add it to npmrc example and it will work. Could you check it?

Scripts with that option works fine on mac

@Pabl0cks
Copy link
Collaborator

After doing some digging I got it working on Windows by first running (needs to be run only once in life) :

pnpm config set shell-emulator=true
after doing this pnpm run hardhat:test worked fine (@Pabl0cks could you please try this once ? )

Yeah, it worked !! Great job @technophile-04 🙌

@carletex
Copy link
Member

Some updates that might affect this PR here: #544 (comment)

@carletex
Copy link
Member

As we discussed, closing this for now. Let's keep the branch tho!

@carletex carletex closed this Nov 22, 2023
@technophile-04 technophile-04 deleted the feat/pnpm branch April 25, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use pnpm instead of yarn
5 participants