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

Migrated the legacy guide Backpressuring in Streams to the learn section #6261

Closed
wants to merge 2 commits into from

Conversation

NextThread
Copy link
Contributor

@NextThread NextThread commented Jan 23, 2024

Fixes #6228

Description

Migrated the legacy guide Backpressuring in Streams to the learn section

Validation

Screenshot 2024-01-23 at 2 24 00 PM Screenshot 2024-01-23 at 2 24 10 PM

Related Issues

Check List

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

@NextThread NextThread requested review from a team as code owners January 23, 2024 08:57
Copy link

vercel bot commented Jan 23, 2024

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

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 24, 2024 5:10pm

Copy link
Contributor

github-actions bot commented Jan 23, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 100 🟢 92 🟢 92 🔗
/en/about 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/about/previous-releases 🟢 98 🟢 96 🟢 92 🟢 92 🔗
/en/download 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/blog 🟢 100 🟢 97 🟢 92 🟢 92 🔗

Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 90%
86.49% (397/459) 77.33% (116/150) 77.77% (70/90)

Unit Test Report

Tests Skipped Failures Errors Time
72 0 💤 0 ❌ 0 🔥 4.497s ⏱️

@AugustinMauroy
Copy link
Member

AugustinMauroy commented Jan 23, 2024

My review:

  • change layout to learn.hbs
  • in code block the language should be js instead of javascript
  • when a code snippet import a nodejs internal package we should use the node: like in api docs
  • you can remove <!-- eslint-disable indent -->
  • https://r.va.gg/2014/06/why-i-dont-use-nodes-core-stream-module.html is a death link
  • https://github.com/nodejs/node/blob/55c42bc6e5602e5a47fb774009cfe9289cb88e71/lib/_stream_writable.js#L239 this link is talking about an old code of node. it's worth seeing if it's necessary to talk about it ?
  • I can't load http://dtrace.org/blogs/about/ so I think it's https://dtrace.org/about/ (need to verify)

@AugustinMauroy
Copy link
Member

Wouldn't it be better to put it in the "module" section/category?

Copy link
Collaborator

@bmuenzenmeyer bmuenzenmeyer left a comment

Choose a reason for hiding this comment

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

thanks for starting this work!

as noted in #6228 - can you lightly update the content? here's a couple short observations

  • import vs require
  • prefer node: imports when an internal package
  • streams now have a promise api, negating the need for promisify

@anonrig

This comment was marked as resolved.

@AugustinMauroy

This comment was marked as resolved.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -80,6 +80,12 @@
"modules": "Modules",
"publishingNodeApiModules": "How to publish a Node-API package"
}
},
"backpressuring": {
Copy link
Member

Choose a reason for hiding this comment

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

The category should be "streams", not "Backpressuring". Can you make these changes? (Since it will require changes in many files?)

Also, the link should probably be /learn/streams/backpressuring-in-streams

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this guide should be in the "Modules" category 🤔

@ovflowd
Copy link
Member

ovflowd commented Feb 12, 2024

Can you setup a redirect from the old guide (guides) to the learn page as done in here?

@paulobelucci
Copy link
Contributor

paulobelucci commented Feb 19, 2024

Hi everyone! I'm following this discussion and I opened a new PR #6352 to address the points you guys pointed here.

@ovflowd
Copy link
Member

ovflowd commented Feb 21, 2024

Closing as superseded by #6352

@ovflowd ovflowd closed this Feb 21, 2024
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.

Migrate the legacy guide "Backpressuring in Streams" to the learn section
8 participants