-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: main
Are you sure you want to change the base?
Changes from all commits
a9c88e7
972c040
e88391d
4381bc2
5f149da
abcd8f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
#!/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 | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a little clunky, but my experiment with using process substitution |
||
|
||
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 .npmrc; then | ||
echo "Creating .npmrc..." | ||
echo -e "engine-strict = true\nauto-install-peers = false" > .npmrc | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
fi | ||
|
||
echo "Installing dependencies with pnpm..." | ||
pnpm install | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 What do you think @CvX @tyb-talks ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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!" |
There was a problem hiding this comment.
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 😢
There was a problem hiding this comment.
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 😅 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should 'just work', right? As long as you change the shebang at the top of the file to something like
or for ruby
?
There was a problem hiding this comment.
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.