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

feat: add streams guide #7123

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: add streams guide #7123

wants to merge 3 commits into from

Conversation

Ceres6
Copy link

@Ceres6 Ceres6 commented Oct 16, 2024

Description

This PR adds a stream guide for the learn section

Validation

Lint passing and checked visually

Related Issues

Closes nodejs/node#8646

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Co-authored-by: Matteo Collina <[email protected]>
@Ceres6 Ceres6 requested a review from a team as a code owner October 16, 2024 08:25
Copy link

vercel bot commented Oct 16, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Nov 14, 2024 7:47am

@Ceres6
Copy link
Author

Ceres6 commented Oct 16, 2024

I'm missing translation for the titles as I didn't want to use translator ones

package.json Outdated Show resolved Hide resolved
@AugustinMauroy
Copy link
Member

I'm missing translation for the titles as I didn't want to use translator ones

You mustn't touch to translation fille it's handle with crowdin

@Ceres6
Copy link
Author

Ceres6 commented Oct 16, 2024

I'm missing translation for the titles as I didn't want to use translator ones

You mustn't touch to translation fille it's handle with crowdin

So should I remove the en.json change?

@AugustinMauroy
Copy link
Member

I'm missing translation for the titles as I didn't want to use translator ones

You mustn't touch to translation fille it's handle with crowdin

So should I remove the en.json change?

I mean you just have to update en.json and don't touch to translated file

@Ceres6
Copy link
Author

Ceres6 commented Oct 21, 2024

cc @mcollina @Trott

@mcollina
Copy link
Member

Can you please add a sentence at the beginning or end saying that this guide is a derivative of https://blog.platformatic.dev/a-guide-to-reading-and-writing-nodejs-streams?

@simoneb
Copy link

simoneb commented Oct 22, 2024

@mcollina do we want any corporate references to material that lands on the Node.js website? Asking because, in a similar vein, the article was reviewed by multiple people at Nearform, including myself, and we weren't going to ask @Ceres6 to attribute us.

On the other hand, we were considering to publish the very same article on our company's blog, with a reference to the official documentation, as the authoring has in practice being supported by us.

@mcollina
Copy link
Member

@mcollina do we want any corporate references to material that lands on the Node.js website? Asking because, in a similar vein, the article was reviewed by multiple people at Nearform, including myself, and we weren't going to ask @Ceres6 to attribute us.

@simoneb You can see there are very few original contributions to what has been added here compared to what I wrote in the article, as multiple paragraphs were taken verbatim, including the whole of the introduction.

By all intents and purposes, I'm the primary author of this PR, and I gave permission to use my original piece if I was included as a co-author of it and kindly asked for a backlink if possible.

Adding a backlink is not unreasonable to ask. It's also ok if it's not added, but I would prefer it.


do we want any corporate references to material that lands on the Node.js website?

I think the question for the @nodejs/tsc is:

Should we add a backlink in case existing material is used in the Learn section of the website?

(We should not be using existing material without the author permission anyway).

@mcollina
Copy link
Member

mcollina commented Oct 25, 2024

Here is a proposal, how about you include in the commit description:

  1. a link of the original piece
  2. a mention of all people at NearForm that reviewed it

So we keep a record of the origin of this content.

@Ceres6
Copy link
Author

Ceres6 commented Oct 25, 2024

@mcollina I'm happy to add the backlink. I'm guessing now the TSC is tagged we should wait until that gets discussed, right?

EDIT: I saw that some resources in the learn section have an authors frontmatter prop, maybe that's another option to consider?

@mcollina mcollina removed the blocked label Oct 25, 2024
@mcollina
Copy link
Member

I think it's good if the TSC discuss this because I suspect it would come out more and more.

@Ceres6 you should definitely fill in the authors block in the frontmatter. It should appear something like:

Screenshot 2024-10-25 at 17 59 53

@Ceres6
Copy link
Author

Ceres6 commented Oct 25, 2024

Cool! I'll add authors then and wait for the TSC discussion on the backlink. Just one doubt, should I add Nearform reviewers as authors or are those not considered as such? @mcollina

@mcollina
Copy link
Member

I would list them all.

@ovflowd
Copy link
Member

ovflowd commented Oct 28, 2024

Hey folks 👋 are we happy here?

Copy link

github-actions bot commented Oct 28, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟠 82 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 99 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 99 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 100 🟢 92 🔗

Copy link

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 92%
90.54% (594/656) 76.27% (180/236) 94.35% (117/124)

Unit Test Report

Tests Skipped Failures Errors Time
134 0 💤 0 ❌ 0 🔥 5.157s ⏱️

@Ceres6
Copy link
Author

Ceres6 commented Oct 29, 2024

Hey @ovflowd, I think we're waiting on the next TSC meeting for the backlink issue.

@mcollina
Copy link
Member

@ovflowd do you know if we have a policy for content that was published elsewhere and brought in? That's what is missing in this case.

@ovflowd
Copy link
Member

ovflowd commented Oct 29, 2024

@ovflowd do you know if we have a policy for content that was published elsewhere and brought in? That's what is missing in this case.

The only content-related policy we have is this one https://github.com/nodejs/nodejs.org/blob/main/CONTENT_VS_CODE.md

Nothing else, such as content sourced from different places.

@Ceres6
Copy link
Author

Ceres6 commented Nov 6, 2024

@mcollina Should I add the backlink so we can merge this and we can revisit it if the tsc votes otherwise?

@bmuenzenmeyer bmuenzenmeyer added the learn Issues/pr concerning the learn section label Nov 8, 2024
@mhdawson
Copy link
Member

My opinion is that we should include something like "This content was originally shared by Y in X"

@mhdawson
Copy link
Member

I think we should also document in the collaborator guide for the Node.js website that we should include references when pulling in content from other sources. That process should also be to make sure we document that the original author is ok with the content being pulled in, possibly through approving the PR that pulls it in.

Add authors and original content attribution

Signed-off-by: Carlos Espa <[email protected]>
@Ceres6
Copy link
Author

Ceres6 commented Nov 14, 2024

Authors and original attribution added. Are we good to go, @mcollina?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
learn Issues/pr concerning the learn section tsc-agenda
Projects
None yet
Development

Successfully merging this pull request may close these issues.

doc: guide/topic on how to use streams
7 participants