-
Notifications
You must be signed in to change notification settings - Fork 52
Remove resolvePm2 and fix pm2 types
#2174
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
Conversation
📊 Performance Test ResultsComparing 40fc12d vs trunk site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change |
nightnei
left a comment
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.
Nice change and works as expected 👍
Only one NIT thing - I would avoid formatting changes in Studio's diff and in proposed PR to PM2 (or create a separate follow-up PR with formating), so that it's much easier to review and to see exact changes at a glance and maybe it will speed up review of the PR in PM2.
| const processDescriptions = ( processes || [] ).map( ( p ) => ( { | ||
| name: p.name || '', | ||
| pmId: p.pm_id || -1, | ||
| pmId: p.pm_id ?? -1, |
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 unrelated to the core purpose of this PR, but it's an issue I ran into recently: if p.pm_id is 0, we would assign it a value of -1, which is incorrect.
katinthehatsite
left a comment
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.

Related issues
Proposed Changes
This PR changes how we import pm2. pm2 exports two members:
default(an instantiatedAPI) andcustom(the originalAPIclass). By importingcustomand passing thepm2_homeconfig prop, we can remove theresolvePm2logic.The only problem is that the
custommember isn't accounted for in the pm2 type definitions. I've submitted a PR Unitech/pm2#6064 to pm2 to fix this. Until that's merged, I've updated our pm2 patch to include my changes from that PR.Tip
View the upstream PR diff with whitespace changes hidden to see that I haven't actually changed all that much in
types/index.d.tsTesting Instructions
npm run cli:buildnode dist/cli/main.js site listPre-merge Checklist