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

Use nmMode: hardlinks-global to reduce disk space consumption #5011

Closed
wants to merge 1 commit into from

Conversation

Philzen
Copy link
Contributor

@Philzen Philzen commented Apr 3, 2022

This could alleviate some of the perceived pain of pnpm-fanfolks (like me) when starting their RedwoodJS journey.

It saves ~0.8 GB of disk space for any additional fresh yarn install of a redwood project and also, (at least in theory) is faster as those packages will be linked rather than copied into the project folder.

I've used this setting in an existing project for a couple of weeks now and could not observe any negative side-effects. However, leaving this off to the RedwoodJS dev team to review and decide whether you'd like to merge this enhancement.

@netlify
Copy link

netlify bot commented Apr 3, 2022

Deploy Preview for redwoodjs-docs canceled.

Name Link
🔨 Latest commit e556d58
🔍 Latest deploy log https://app.netlify.com/sites/redwoodjs-docs/deploys/6249a755285b610009ff9716

@jtoar jtoar self-requested a review April 3, 2022 14:02
@jtoar jtoar self-assigned this Apr 3, 2022
@thedavidprice thedavidprice marked this pull request as draft April 3, 2022 16:54
@thedavidprice
Copy link
Contributor

@Philzen thanks for this! Give us a week to let the dust settle on v1 release. But do help us keep this conversation going after that.

@jtoar (not urgent) I forget why the Yarn team recommended "local" Overall, in a couple weeks you and I should probably revist the overall status for deps and plot a course for where we want to go.

@jtoar
Copy link
Contributor

jtoar commented Apr 5, 2022

@thedavidprice here's relevant comments about the nmMode: hardlinks-local setting:

The first one I linked to is probably why we decided on nmMode: hardlinks-local at the time.

@thedavidprice
Copy link
Contributor

@jtoar Ah, there you go. And, correct, I don't believe we can default to global without incurring very hard to explain issues for some projects.

But how can we make devs and projects "aware" of performance optimizations like this? Maybe start a new Yarn doc where we highlight commands like yarn dedupe and advanced performance config like this? We could link to the doc from the rc file.

Reactions?

@jtoar
Copy link
Contributor

jtoar commented Apr 9, 2022

But how can we make devs and projects "aware" of performance optimizations like this? Maybe start a new Yarn doc where we highlight commands like yarn dedupe and advanced performance config like this? We could link to the doc from the rc file.

Yeah a great start would probably be commenting the .yarnrc.yml file.

The hard thing about this is it's mostly uncharted territory. Like I'd say you can't touch the nodeLinker setting, but I'm really not sure about the others. So experimentation is super important (thanks @Philzen!)

@jtoar
Copy link
Contributor

jtoar commented Apr 9, 2022

@Philzen thanks for trailblazing here! We still have a lot of exciting experiments to do with yarn. I think it'll be unlikely we change the default behavior right now, but I did add comments to the file here to let users know that they're free to tweak some of the options. What do you think? #5093

@jtoar jtoar removed their assignment Apr 15, 2022
@thedavidprice
Copy link
Contributor

Hi all, just checking back on the status of this one.

@jtoar
Copy link
Contributor

jtoar commented Apr 19, 2022

Going to close this in favor of #5093 and #5249. Thanks @Philzen for all the feedback!

@jtoar jtoar closed this Apr 19, 2022
@Philzen Philzen deleted the patch-1 branch April 19, 2022 20:42
@Philzen Philzen restored the patch-1 branch April 19, 2022 20:42
@Philzen Philzen deleted the patch-1 branch April 19, 2022 20:42
@Philzen
Copy link
Contributor Author

Philzen commented Jul 10, 2022

@thedavidprice

I think it'll be unlikely we change the default behavior right now, but I did add comments to the file here to let users know that they're free to tweak some of the options. What do you think? #5093

I love the @jtoar's PR adding that info in #5093. However, there is still a chicken-egg problem here: You cannot directly create a redwood project from scratch and benefit from the disk space savings of hardlinks-global on the first go. Therefore the DX for that is a little inconvenient:

  1. create the project (for which you'll have need additional ~0.8 GB disk space)
  2. then cd into the created project's folder and change your .yarnrc file to nmMode: hardlinks-global
  3. rm -rf node_modules (takes a couple of seconds even on an SSD)
  4. run yarn install again

For the ideal DX we'd save people the time and disk space involved in steps 1 – 3. I could see two options going forward to alleviate that:

A. make nmMode: global-hardlinks the default on the next major version bump
B. provide a short and simple CLI option for yarn create-redwood-app (we may want that for the other way around in case we go for option A.)

Would love to hear your perspectives on this, and ideally track of option B. in a separate RFC.


I'm trying to understand the caveat / implications that lead to the decision to close this PR – please add / correct me if i'm overlooking something here: As far as i my understanding goes, if this became the default, it would mean if I changed some code in the dist-files of a package contained in my node_modules folder of project A, that change would apply in project B as well (which should be the same behaviour in pnpm).
I'd understand that some people would consider this a serious heads-up / caveat / dangerous black magic as the default behaviour. No matter whether A or B is going forward, it's of course wise to have that clearly documented in #5249.

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.

3 participants