Vue
May 2024 - Vue core PR queue #11010
Replies: 2 comments 3 replies
-
I guess it's @shengxinjing's account. He said on Twitter/X that his account somehow got banned by GitHub: https://x.com/shengxj1/status/1788129722951680278 |
Beta Was this translation helpful? Give feedback.
-
|
Yes, it was a PR created by me. I don’t know why my github account was suddenly banned. Now I use this account and it will probably be restored after github’s appeal is passed. |
Beta Was this translation helpful? Give feedback.
-
|
@sxzz was helping out on core reviews but he is now mostly focusing on Vapor. I have to split my attention between Vite / Rolldown and Vue core, and usually I allocate a "focus cycle" to burn down issues / PRs. Many of the hanging PRs were probably submitted during periods where I was entirely focusing elsewhere. I did ask team members to help with core review when I'm not working on Vue core, but it seems not many have the bandwidth and required knowledge / context to do so. We definitely could use some more active help on core reviews. When I do get to Vue core, the natural tendency is to focus on the newer PRs due to fresher context, or higher priority ones (p3 and above). I'll be completely honest I dreaded going through old PR lists because I worry I won't have energy left to actually work on them at the end of the focus cycle - so I really appreciate you going through these and surfacing easy / valuable-but-overlooked PRs. I think I can be much more efficient if I have a list of such PRs at the beginning of a Vue core focus cycle. A structured list ranked by priority / easiness of merging, even only based on your opinion, would be very very helpful. I also like the suggestions of starting with active team members' PRs - this is something I should definitely do. And finally, I won't worry too much about merge conflicts as I have gotten pretty good at resolving them. 😅 Again, thanks for the thoughtful post and suggestions. |
Beta Was this translation helpful? Give feedback.
-
I've started work on this. I should be able to create another shortlist of easy wins in a few days, then I'll see what I can do with the more complex ones. |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
Back in October 2023, I shared some thoughts about the state of the Vue core PR queue. 7 months on, I figured it might be worth re-running the analysis to see how things have changed.
My original post is at:
I'm not going to repeat everything I said there. If you aren't familiar with the previous discussion I suggest reading that first, as it covers some important topics (e.g. why this matters) that I'm not going to repeat here.
My original analysis used data from October 19, 2023. The new analysis is from May 19, 2024. As previously, I've ignored PRs opened by bots, those marked as drafts, and those opened in the current month.
Some headline figures:
That's undoubtedly a significant improvement - about 30% smaller. Clearly there's a lot of work gone into making that happen, so a big thank you from me to everyone involved.
The charts
As previously, I split the PRs into 3 groups: open, merged and closed. I then plotted
M / (M + O), grouped by the month they were opened:To learn more about what exactly those charts are showing, see https://gist.github.com/skirtles-code/a402e5008a38d08cd3ae8020656ca752.
At first glance, this doesn't look great for Vue. But rather than comparing Vue to other repos, it might be more constructive to focus on how things have changed in the last 7 months.
This next chart combines the data from May and October into a single chart. The grey bars represent where things were in October and green shows the progress since then:
Maybe it's more useful to stack the bars the other way up, so here's what that looks like:
Some caution is required when interpreting the green portions of those bars. While they might represent real progress, such as the merging of PRs, it could equally indicate that a PR was closed because the author deleted their fork, even though the changes were perfectly good. There wasn't an easy way for me to indicate that on the charts. All it really indicates is how much closer we are to having no open PRs for that month.
I'd encourage you to come up with your own interpretation of all these charts before reading mine...
My main thoughts are:
Are the old PRs still mergeable?
Merge conflicts are a significant obstacle to reviewing and merging old PRs. How many of the open PRs have conflicts? The chart below attempts to help answer that question. It shows the number of open PRs for each month, with colours indicating how many have conflicts and how many don't:
As expected, the further back in time we go the more conflicts we see.
But arguably it's more interesting that there are PRs from 2021 and 2022 that don't have conflicts.
There are a few reasons for that. In some cases it's just because the changes are really small, so a conflict would only be introduced if a specific line changed. In other cases, it's because the original author has kept resolving the conflicts over the last couple of years to ensure that their PR is still mergeable. Kudos to
baiwusanyu-candedison1105in particular.I think it would be really helpful if PRs were automatically assigned a label to indicate whether they have a conflict. For more about that idea, see:
Are the old PRs junk?
That's complicated.
If a PR is obviously junk then it's relatively easy to close it. I don't think there are many left like that.
The most difficult PRs to handle are ones that have merit but need lots more work reviewing and iterating before they can be merged. Those can suck up huge amounts of time for fixes that aren't necessarily important.
So are all the old PRs like that? Not really.
I recently read through all the open PRs, just to try to understand what's there. It took me about a week, and that was just to read through them, not to review them. The job of reviewing all of those properly is huge. But there are still plenty of small changes that can be reviewed relatively quickly.
I found quite a few that could just be closed. I reported some of those in #10770, most of which are now closed.
But there are quite a lot of other 'easy' ones. Take these, for example:
getSSRPropstype argument #5691effect#7664Object.assignwithextend#8988Some of these could be merged, some should just be closed, but either way it doesn't seem too difficult to reach a decision.
The current process/strategy
I'm not exactly sure what the current process/strategy is for trying to work through the old PRs. I can see some work from
sodateain recent weeks, in particular adding labels to old PRs. It also looks like https://github.com/orgs/vuejs/projects/13 is being updated.It wasn't clear to me whether anyone is actually doing any reviewing though. I'm not trying to point fingers, people are busy and the work is entirely voluntary... but, in practice, is the current process actually working?
When I wrote my analysis in October, I felt that the main bottleneck was on Evan doing the merging. I think that was a fair assessment at the time, but I'm not sure whether that's still true. I could be totally off base here, but it seems that right now there's a problem further upstream with getting things reviewed. It is very hard to judge, though.
I also wonder how much value most of those labels really add. Does labelling a PR as
p1,p2,p3, etc. actually make any difference? I mean, if a PR is already 12 months old, does it really matter whether it'sp1orp3? Likewise, does theready for reviewlabel have any real impact? I get the theory, but is it working in practice?I quite liked the
easy for mergelabel. That seemed like a nice idea, but it seems to have fallen into disuse.Suggestions for reviewers
Suggestion 1
One reason reviewing an old PR is difficult is that the original author is no longer interested in making updates. You could spend hours writing feedback, only for it to be ignored.
If that's putting you off doing reviews, I have a suggestion: just review PRs from people you know.
baiwusanyu-chas 31 open PRs.edison1105has 26,Alfred-Skybluehas 25. That's 82 PRs where you can be confident that the original author is still interested.Is that unfair on everyone else, like a form of nepotism? Yes it is, but I think it's a pragmatic way to make some progress towards clearing the PR queue. I know it helps me to stay motivated when I'm reviewing a PR from a team member.
Suggestion 2
I know it's tempting just to review the new PRs and ignore the old ones. It's also tempting to skim through the list and only review PRs that sound interesting.
When I went through all the open PRs, I started at the end of the queue, with the open PRs from 2019. Some PRs I only looked at very briefly, but I forced myself to make notes about each PR. That ensured that I had understood roughly what it was about before moving on.
I found this quite an effective approach. By forcing myself to read through all the old PRs it allowed me to find lots of interesting PRs that I would never have looked at just based on their titles.
As I mentioned earlier, I think it would be beneficial if the PRs were automatically labelled to indicate whether they have merge conflicts. I think reviewing the oldest PRs first is a good (and fair) approach to reducing the size of the queue, but it would be more effective if we could filter the list to ignore PRs with conflicts.
Other ideas
Perhaps we need to take a slightly more aggressive approach towards closing old PRs?
There are 3 open PRs from 2019, 5 from 2020, and 26 from 2021. Perhaps some still have merit, but surely most of them are never going to be merged. If there are valuable ideas in those PRs, perhaps an issue should be opened instead so the PR can be closed?
Maybe PRs with merge conflicts should be closed? I think the automatic labelling could help with that. Closing the PRs has the potential to really annoy the original authors, so perhaps it could be done in stages: first posting a comment asking them to resolve the conflicts, then closing the PR if they aren't resolved within 2 weeks? I realise we may lose some valuable PRs to this strategy, but it might be worth it to reduce the size of the queue, especially given the difficulty in reviewing/merging PRs with conflicts.
Another idea, and I know this may sound nuts, but would it be helpful if those of us with old open PRs just closed them and opened new PRs to replace them? It seems the review process works well for new PRs, but struggles to work through the old PRs. So maybe it'd be easier just to 'bump' PRs by re-opening them?
A tangent
While compiling the comparison between October and May, I noticed some PRs had disappeared. They aren't just closed, they're gone altogether. They were all created by the same user, who seems to have had their account deleted. For reference, these are the 'vanished' PRs:
GitHub still shows their titles and merge status, but if I click on them, I get a 404. Perhaps they are still visible to those with higher permissions, I don't know. I found it interesting that PRs can be deleted like this.
They didn't significantly impact the charts, so I didn't attempt to correct for their disappearance.
I actually have the diffs for the 5 'open' PRs in an old data dump, so I may take a look at some point to see whether they're worth salvaging.
Thanks for reading.
Beta Was this translation helpful? Give feedback.
All reactions