chore: add post-merge hook #2025
Conversation
SiriusCrain
requested review from
afonsojramos,
bmulholland and
setchy
as code owners
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| "node": ">=22" |
There was a problem hiding this comment.
👀 good catch @SiriusCrain - how about we split this out into its own PR?
I'm still thinking about whether we want that post-merge hook...
There was a problem hiding this comment.
I can upgrade node in another PR, sure, post-merge hook from my experience can help a lot to avoid bugs or errors about missing package etc.
There was a problem hiding this comment.
@setchy reverted package.json changes btw
There was a problem hiding this comment.
Feel free to share your thoughts or close PR if you want to
There was a problem hiding this comment.
@setchy what do you think? Should we move further with this decision?
There was a problem hiding this comment.
I think for now I'd like to leave this out, but keen to hear what @afonsojramos thinks 👂
ChatGPT summarize this as follows, rightly or wrongly
| Setup | Good Idea? | Notes |
|---|---|---|
pnpm install --force always |
🚫 | Too aggressive, slow, not always needed |
pnpm install always |
Better, but still wasteful if deps didn’t change | |
| Conditional install | ✅ | Best balance of correctness and performance |
There was a problem hiding this comment.
Got it, I will close PR then
SiriusCrain
force-pushed
the
chore/add-post-merge-hook
branch
from
8a8a14a to
2c04f84
Compare
2 participants
When we are updating local branch I think it is crucial to keep dependencies up to date, to avoid confusions or errors