-
Notifications
You must be signed in to change notification settings - Fork 78
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
Future of llnode #355
Comments
Adding this to the agenda so we can discuss it further tomorrow. |
mmarchini
added a commit
to mmarchini/llnode
that referenced
this issue
Feb 12, 2020
As discussed in the 2020-02-12 Diagnostics WG Meeting and suggested in nodejs#242 (comment), change the landing policy so that Collaborators can land pull requests if it was open for three days and there were no reviews on it. Ref: nodejs/diagnostics#355
mmarchini
added a commit
to nodejs/llnode
that referenced
this issue
Feb 22, 2020
As discussed in the 2020-02-12 Diagnostics WG Meeting and suggested in #242 (comment), change the landing policy so that Collaborators can land pull requests if it was open for three days and there were no reviews on it. Ref: nodejs/diagnostics#355 PR-URL: #336 Refs: nodejs/diagnostics#355 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Discussed in last Diagnostic WG meeting. Required changes made to landing policy, closing. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I've raised this topic before, but I think it's still a problem, so I'm raising it again, now with a slightly different perspective.
llnode is moving slow. Slower than it should. And slower than it's healthy for the project. For llnode to stay relevant, it needs to keep up with V8 and Node.js, two fast paced projects. It should minimally keep up with Node.js LTS releases.
llnode doesn't have many contributors, which is understanding. The project is complex and requires frequently reading V8 source code. We tried to promote the project in the past to get more attention. It didn't work. At most we got some style-related (linter, etc.) contributions, which are not very helpful in our case because it only increases the burden on current maintainers to review. Which brings us to the next point.
Reviewers frequency fell over the past two years. The current state of things is (mostly): one person contributing with code, another person reviewing code in
src/
, and another person reviewing build-related code. Since the project is not a priority to most folks, reviews will take time. For small PRs, reviews might come faster. But for larger PRs it can take months and several pings.I noticed that trend, so for the v12 currency I tried to break all changes into the smallest PRs possible, to make review easier. Still took a while to get review on the first few PRs, and then reviews stopped coming. In the end I gave up, closed the small PRs, opened one huge PR with all remaining changes for v12 currency, and escalated the reviews request to the WG. While this worked, it's not sustainable to escalate every PR to the WG. It also doesn't make much sense, since most folks here are not familiar with llnode's code base.
In the current state, the whole review process ends up being demotivating and draining. I proposed this before, but I think we should drop the review process for llnode, as it's not really adding anything meaningful for the project and is preventing it from moving forward.
I'm proposing this again because I got my motivation to work on llnode back and have some extra cycles I can use for that. There are many things I want to do which will improve the project reliability and usability (for example: add ASAN/UBSAN builds to our CI, standardize output, move JS API out of experimental, allow accessors with property names and array indexes, etc.). I also intend to keep llnode up to speed with at least Node.js master.
If removing reviews is not possible, I'll likely fork the project (either under
@mmarchini/llnode
or a new name on npm) so it can move faster.The text was updated successfully, but these errors were encountered: