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.

Sign up for GitHub

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

Jump to bottom

doc: Use readFileSync instead of fs.readFileSync in ALPN sample #15137

Closed
wants to merge 1 commit into from
Closed

doc: Use readFileSync instead of fs.readFileSync in ALPN sample #15137

wants to merge 1 commit into from

Conversation

miensol
Copy link
Contributor

@miensol miensol commented Sep 1, 2017

ALPN negotiation sample uses fs.readFileSync. However readFileSync
is imported into top level variable and should thus be used instead.

Checklist
Affected core subsystem(s)

doc

@miensol
Use readFileSync instead of fs.readFileSync
7fad0eb 
The ALPN negotiation sample uses `fs.readFileSync` whereas `readFileSync` is imported into top level variable.
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Sep 1, 2017
benjamingr
benjamingr approved these changes Sep 1, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for making a contribution!

I have approved it for you and if no one objects for 72 hours (to give collaborators ample time to respond) it will be landed.

@jasnell
Copy link
Member

jasnell commented Sep 1, 2017

(btw, I'm not sure this needs to wait the full 48/72 hours to land)

@refack
Copy link
Contributor

refack commented Sep 1, 2017

  1. Small doc change
  2. 5 approvals (2 TSC members)
  3. 4 👍 for fast track (2 TSC members)
  4. passes local mdlint

Landing

refack pushed a commit that referenced this pull request Sep 1, 2017
@miensol @refack
doc: readFileSync instead of fs.readFileSync
ec599b8 
The ALPN negotiation sample uses `fs.readFileSync` whereas
`readFileSync` is imported into top level variable.

PR-URL: #15137
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@refack
Copy link
Contributor

refack commented Sep 1, 2017

Landed in ec599b8

@miensol congratulations on your first landed contribution. GitHub has promoted you from:
image
to
image
🍾

@refack refack closed this Sep 1, 2017
@eljefedelrodeodeljefe
Copy link
Contributor

Not sure what happened here. Is destructuring core module methods a thing? Why is it inconsistent now? Should it not have been rather fixing it at require level?

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@miensol @addaleax
doc: readFileSync instead of fs.readFileSync
db8f8d7 
The ALPN negotiation sample uses `fs.readFileSync` whereas
`readFileSync` is imported into top level variable.

PR-URL: nodejs/node#15137
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@miensol @MylesBorins
doc: readFileSync instead of fs.readFileSync
468631b 
The ALPN negotiation sample uses `fs.readFileSync` whereas
`readFileSync` is imported into top level variable.

PR-URL: #15137
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@miensol @MylesBorins
doc: readFileSync instead of fs.readFileSync
9542844 
The ALPN negotiation sample uses `fs.readFileSync` whereas
`readFileSync` is imported into top level variable.

PR-URL: #15137
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@targos targos targos approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@miensol @jasnell @refack @eljefedelrodeodeljefe @benjamingr @targos @vsemozhetbyt @MylesBorins @nodejs-github-bot