Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
DEV: add script to switch from yarn to pnpm #11
Changes from 2 commits
a9c88e7
972c040
e88391d
4381bc2
5f149da
abcd8f0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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.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.
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?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.
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.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.
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 ?
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.
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.