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

DEV: add script to switch from yarn to pnpm #11

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions scripts/switch-yarn-to-pnpm.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
#!/bin/bash
set -euxo pipefail

# Helper functions and setup of jq
file_exists() {
[ -f "$1" ]
}

command_exists() {
command -v "$1" >/dev/null 2>&1
}

if ! command_exists jq; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore but... I wonder if we should just bite the bullet and write this script in Ruby? I made the mistake of writing the original ones in bash cos I thought they would be simple... but complexity has ballooned over time 😢

Copy link
Author

@tyb-talks tyb-talks Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe for a future one? I'm still finding the current use cases ok for bash. (of course, without adding tools like jq in bash, it becomes quite hairy to handle stuff like json 😅 )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in Ruby

some time ago I started redoing existing scripts in node js 😅 we could revive that effort, but it's low priority. though we could at least land the support for js scripts (w/o actually finishing the rewrite of all of the existing stuff)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

land the support for js scripts

It should 'just work', right? As long as you change the shebang at the top of the file to something like

#!/usr/bin/env node

or for ruby

#!/usr/bin/env ruby

?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, I suppose that would work too. I was thinking about "native" support, without shelling out, but I guess that would make more sense when all scripts are already in js.

echo "jq is not installed. Attempting to install it..."
if command_exists apt-get; then
sudo apt-get update && sudo apt-get install -y jq
elif command_exists brew; then
brew install jq
else
echo "Error: Cannot install jq. Please install it manually and run this script again."
exit 1
fi
fi

cd repo

if ! file_exists package.json; then
echo "ERROR: 'package.json' is missing. Cannot proceed with conversion."
exit 1
fi

if file_exists yarn.lock; then
echo "Removing yarn.lock..."
rm yarn.lock
fi

if file_exists package-lock.json; then
echo "Removing package-lock.json..."
rm package-lock.json
fi

echo "Replacing 'yarn' with 'pnpm' in package.json scripts..."
jq 'if has("scripts") then .scripts |= with_entries(.value |= gsub("yarn "; "pnpm ")) else . end' package.json > temp.json && mv temp.json package.json
Copy link
Author

@tyb-talks tyb-talks Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a little clunky, but my experiment with using process substitution cp package.json <(jq 'some filter' package.json) errored out, so I think the temp file way is a more consistent across machines.


echo "Updating engines clause in package.json..."
jq '.engines = (.engines // {}) |
.engines *= {
"node": ">= 18",
"npm": "please-use-pnpm",
"yarn": "please-use-pnpm",
"pnpm": ">= 9"
}' package.json > temp.json && mv temp.json package.json

if ! file_exists .pnpmfile.cjs; then
echo "Adding .pnpmfile hook to clean up yarn managed node_modules..."
cat << 'EOF' > .pnpmfile.cjs
const fs = require("fs");
const { execSync } = require("child_process");

const currRoot = __dirname;

if (fs.existsSync(`${currRoot}/node_modules/.yarn-integrity`)) {
console.log(
"Detected yarn-managed node_modules. Performing one-time cleanup..."
);

// Delete entire contents of all node_modules directories
// But keep the directories themselves, in case they are volume mounts (e.g. in devcontainer)
execSync(
`find ${currRoot}/node_modules -mindepth 1 -maxdepth 1 -exec rm -rf {} +`
);

console.log("cleanup done");
}
EOF
fi

if ! file_exists .npmrc; then
echo "Creating .npmrc..."
echo -e "engine-strict = true\nauto-install-peers = false" > .npmrc
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same npmrc settings we have for core - would be needed for ensuring running pnpm install locally on a theme/plugin repo behaves the same way.

fi

echo "Installing dependencies with pnpm..."
pnpm install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a cleanup script for existing setups, like we have in core?

https://github.com/discourse/discourse/blob/main/.pnpmfile.cjs#L6C1-L19C1

That is: will pnpm i work in dev envs that have yarn-created node_modules dirs?

Copy link
Author

@tyb-talks tyb-talks Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried without the cleanup script, pnpm tries to move the old modules into a .ignored folder, but I think for a cleaner slate, it's better to add this anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I added it to core was because pnpm's auto cleanup doesn't work in workspaces. Themes/plugins are much simpler, so I think it's probably fine to just let pnpm do its thing, and avoid duplicating the .pnpmfile hack in every single repo.

What do you think @CvX @tyb-talks ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pnpm's auto cleanup doesn't work in workspaces

Ah I see.. in that case, I'm ok to let pnpm do its thing.. worst case, devs can rm their local node_modules and reinstall with pnpm.


echo "Updating README with pnpm"
for file in README.md README; do
if [ -f "$file" ]; then
echo "Replacing 'yarn' with 'pnpm' in $file..."
sed -i '' 's/yarn/pnpm/g' "$file"
fi
done

echo "Conversion complete!"