Skip to content
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

docs: add Ada to the dependencies page #5185

Closed
wants to merge 5 commits into from
Closed

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 23, 2023

The current dependencies list is not up to date. The following changes only updates Ada, Node.js's new URL parser.

@anonrig anonrig requested a review from a team as a code owner March 23, 2023 19:27
@anonrig anonrig enabled auto-merge (rebase) March 23, 2023 20:14
@anonrig anonrig requested a review from ovflowd March 23, 2023 20:33
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list is very incomplete. Higher-impact dependencies such as undici and nghttp2 are not listed either. I am not sure what subset of dependencies makes sense here, or if this page provides value at all in its current state.

@ovflowd
Copy link
Member

ovflowd commented Mar 23, 2023

@tniessen the page is just super outdated. But would love if it can be updated with all the new stuff :D

@anonrig anonrig disabled auto-merge March 23, 2023 21:58
@anonrig anonrig enabled auto-merge (squash) March 23, 2023 21:58
@Trott
Copy link
Member

Trott commented Mar 24, 2023

I'm not sure this page adds value for the average user. I wonder if it should be ported to a README for the deps directory in Node.js source. Then we can replace the page with a link there. That is far more likely to be updated when people add/remove dependencies. And because the README is associated with a particular commit, it will reflect the status at that commit. In other words, we avoid the problem of trying to write a single page that reflects all versions of Node.js.

@Trott
Copy link
Member

Trott commented Mar 24, 2023

I'm not sure this page adds value for the average user. I wonder if it should be ported to a README for the deps directory in Node.js source. Then we can replace the page with a link there. That is far more likely to be updated when people add/remove dependencies. And because the README is associated with a particular commit, it will reflect the status at that commit. In other words, we avoid the problem of trying to write a single page that reflects all versions of Node.js.

None of this is an objection to updating the page to reflect current-ish information though.

@ovflowd
Copy link
Member

ovflowd commented Mar 24, 2023

That's correct, @Trott. We can close this PR and pivot to doing something on node/deps/README.md. I mention this because I remembered the nodejs.dev "about" doesn't mention any of these dependencies.

Meaning even if we add this now, as we agreed that the .dev pages replace the .org ones, it will be removed very prematurely.

@anonrig do you think you can pivot such kind of change on @nodejs/node/deps/README.md?

@ovflowd ovflowd disabled auto-merge March 24, 2023 08:57
anonrig and others added 2 commits March 24, 2023 09:30
Co-authored-by: Rich Trott <rtrott@gmail.com>
Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
Co-authored-by: Rich Trott <rtrott@gmail.com>
Signed-off-by: Yagiz Nizipli <yagiz@nizipli.com>
@anonrig
Copy link
Member Author

anonrig commented Mar 24, 2023

And because the README is associated with a particular commit, it will reflect the status at that commit.

@Trott Even though, I agree with your concerns, this particular recommendation will lead to outdated commit URL in this page too. If we are going this path, I think we should include the dependencies and distribute it with the Node.js documentation (which is version specific). (I didn't had any chance to look into the documentation tools & infrastructure. I don't know the depth or possibility of this task, therefore my assumption/recommendation might be invalid.)

None of this is an objection to updating the page to reflect current-ish information though.

@Trott I recommend merging this pull request, and later discuss the future of this document in a separate PR or an issue for wider visibility.

Meaning even if we add this now, as we agreed that the .dev pages replace the .org ones, it will be removed very prematurely.

@ovflowd If this is the case, we should remove those documents from Crowdin. A lot of people are investing their own time to translate these soon-to-be-removed documents, and if we are simply not use them in the future, we should suspend all translation activities.

@anonrig do you think you can pivot such kind of change on @nodejs/node/deps/README.md?

@ovflowd As a non-native English speaker, I think there are more qualified people who can do this faster & cleaner than me.

@ovflowd
Copy link
Member

ovflowd commented Mar 24, 2023

@anonrig Ive made an announcement on Crowdin several days ago asking people to no translate the current content as it's going to be 100% discarded 🙈

@vercel
Copy link

vercel bot commented Apr 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
nodejs-org ⬜️ Ignored (Inspect) Apr 30, 2023 7:34am
nodejs-org-stories ⬜️ Ignored (Inspect) Apr 30, 2023 7:34am

amantank

This comment was marked as outdated.

@bmuenzenmeyer
Copy link
Collaborator

@ovflowd As a non-native English speaker, I think there are more qualified people who can do this faster & cleaner than me.

I'd be happy to draft this up!

@bmuenzenmeyer
Copy link
Collaborator

I was about to propose a new README at deps/README.md, but a quick scan of the current codebase found an existing and probably better source: doc/contributing/maintaining/maintaining-dependencies

Taking @Trott's words here as a guiding set of principles, this file seems to have everything we would want:

  • I'm not sure this page adds value for the average user. - this is a contributing file. Agree that most users won't even know or care about internals
  • I wonder if it should be ported to a README for the 'deps' directory in Node.js source. - while perhaps it might be nice to duplicate a list or link to the maintaining-dependencies, I'd rather defer to the project maintainers over in nodejs/node that have not yet found reason to create a new README. This is not the case for a directory like https://github.com/nodejs/node/tree/main/test which does have useful and unique info
  • Then we can replace the page with a link there. - would this get too into the weeds of maintaining direct from the website? I'd argue no, as we are already discussing linking directly into GitHub.
  • That is far more likely to be updated when people add/remove dependencies. And because the README is associated with a particular commit, it will reflect the status at that commit. In other words, we avoid the problem of trying to write a single page that reflects all versions of Node.js. - the organic use of doc/contributing/maintaining/maintaining-dependencies already qualifies, citing a recent update like deps: upgrade to libuv 1.46.0 node#48618 that maintained this file

tl;dr: merge this as discused previously, but eventually shift toward linking directly to https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#dependency-list or https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md

@ovflowd
Copy link
Member

ovflowd commented Jul 4, 2023

I think we should delete this page and make the link on navigation to simply redirect to the Markdown file on core.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants