fs: make process.binding('fs') internal by shisama · Pull Request #22478 · nodejs/node · GitHub
Skip to content

fs: make process.binding('fs') internal#22478

Closed
shisama wants to merge 1 commit into
nodejs:masterfrom
shisama:internal-binding-fs
Closed

fs: make process.binding('fs') internal#22478
shisama wants to merge 1 commit into
nodejs:masterfrom
shisama:internal-binding-fs

Conversation

@shisama

@shisama shisama commented Aug 23, 2018

Copy link
Copy Markdown
Contributor

Refs: #22160

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 23, 2018
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 23, 2018
@addaleax

Copy link
Copy Markdown
Member

@addaleax

Copy link
Copy Markdown
Member

@jasnell

jasnell commented Aug 23, 2018

Copy link
Copy Markdown
Member

This one also needs to be added to the fall-through whitelist in internal/bootstrap/node.js

@jdalton

jdalton commented Aug 23, 2018

Copy link
Copy Markdown
Member

@shisama For reference of the allow list changes you can check out this other pr.

@shisama shisama force-pushed the internal-binding-fs branch 3 times, most recently from 121225f to 69f7b28 Compare August 25, 2018 02:52
@shisama

shisama commented Aug 25, 2018

Copy link
Copy Markdown
Contributor Author

Thank you for your reviews. I fixed them.

@shisama shisama force-pushed the internal-binding-fs branch 2 times, most recently from 3dd21e1 to e367e77 Compare August 31, 2018 10:01

@BridgeAR BridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM for version 12.

@BridgeAR BridgeAR added the blocked PRs that are blocked by other issues or PRs. label Aug 31, 2018
jasnell
jasnell previously approved these changes Sep 4, 2018
@jdalton

jdalton commented Sep 4, 2018

Copy link
Copy Markdown
Member

Why the semver-major on this if it's just changing how internally the fs binding is referenced? Externally process.binding('fs') still works the same, right?

@jasnell

jasnell commented Sep 4, 2018

Copy link
Copy Markdown
Member

That's just being very defensive and careful. process.binding() can be... touchy.

@targos

targos commented Sep 4, 2018

Copy link
Copy Markdown
Member

Semver-major makes sense but do we really need to prevent this from being in Node 11? I mean, we do not expect this to break anything and if citgm is fine with it and npm or graceful-fs are not broken, I think it's fine.

@jasnell

jasnell commented Sep 4, 2018

Copy link
Copy Markdown
Member

If citgm (and npm in particular) will continue to work, then I'm +1 on landing this in 11.

@jasnell

jasnell commented Sep 4, 2018

Copy link
Copy Markdown
Member

I don't believe this should be labeled blocked. If there's a disagreement on whether it should land for 11, then let's flag it for tsc-review or tsc-agenda.

@jasnell jasnell dismissed their stale review September 4, 2018 18:22

This is failing in CI

@jasnell jasnell left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are relevant CITGM failures that need to be investigated.

@BridgeAR

BridgeAR commented Sep 5, 2018

Copy link
Copy Markdown
Member

CI https://ci.nodejs.org/job/node-test-pull-request/17032/ (The failure looked like it should also show up in our tests).

@shisama shisama force-pushed the internal-binding-fs branch 3 times, most recently from 8855aa7 to af2a68b Compare September 18, 2018 15:01
@shisama shisama force-pushed the internal-binding-fs branch from af2a68b to b7a5a87 Compare October 17, 2018 01:10
@shisama

shisama commented Oct 18, 2018

Copy link
Copy Markdown
Contributor Author

@jasnell @BridgeAR Thank you for your review and running CI. I rebased this, please run CI again. Thanks.

@joyeecheung

Copy link
Copy Markdown
Member

@shisama Can you squash the commit before landing them? See https://github.com/nodejs/node/blob/7e1b178fb637abc68b1d4da1363a19db7ad02d6c/doc/guides/contributing/pull-requests.md#commit-squashing

@shisama shisama force-pushed the internal-binding-fs branch from 98751dd to 7b2d4a8 Compare November 12, 2018 15:46
@shisama

shisama commented Nov 12, 2018

Copy link
Copy Markdown
Contributor Author

@joyeecheung Thank you. I squashed them.

@shisama shisama force-pushed the internal-binding-fs branch from 7b2d4a8 to 79834c9 Compare November 13, 2018 01:23
@shisama shisama force-pushed the internal-binding-fs branch from 79834c9 to df729bc Compare November 13, 2018 13:14
@shisama

shisama commented Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

@shisama

shisama commented Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

@shisama

shisama commented Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

@BridgeAR Hi, is the label blocked still needed?

@BridgeAR

Copy link
Copy Markdown
Member

I don't think so.

@BridgeAR BridgeAR removed the blocked PRs that are blocked by other issues or PRs. label Nov 13, 2018
@shisama

shisama commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

Resume Build for node-test-commit-arm-fanned: https://ci.nodejs.org/job/node-test-pull-request/18670/

shisama pushed a commit that referenced this pull request Nov 16, 2018
Refs: #22160

PR-URL: #22478
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@shisama

shisama commented Nov 16, 2018

Copy link
Copy Markdown
Contributor Author

Landed in 1e23e3c

@shisama shisama closed this Nov 16, 2018
joyeecheung pushed a commit to joyeecheung/node that referenced this pull request Jan 11, 2019
Refs: nodejs#22160

PR-URL: nodejs#22478
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
Refs: #22160

PR-URL: #22478
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Backport-PR-URL: #25446
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
Refs: nodejs#22160

PR-URL: nodejs#22478
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>

Backport-PR-URL: nodejs#25446
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    nodejs#24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    nodejs#24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    nodejs#25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    nodejs#25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    nodejs#22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    nodejs#25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    nodejs#25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) nodejs#25361

PR-URL: nodejs#25537
BridgeAR added a commit that referenced this pull request Jan 18, 2019
Notable Changes

* compression / zlib:
  * Added brotli support (Anna Henningsen and Zach Vacura)
    #24938
* console:
  * Added `inspectOptions` option (Ruben Bridgewater)
    #24978
* crypto:
  * Always accept private keys as public keys (Tobias Nießen)
    #25217
* deps:
  * Upgrade npm to v6.5.0 (Jordan Harband)
    #25234
* fs:
  * Use internalBinding('fs') internally instead of
    process.binding('fs') (Masashi Hirano)
    #22478
* http(s):
  * Support overriding http\\s.globalAgent (Roy Sommer)
    #25170
* util:
  * Inspect ArrayBuffers contents closely (Ruben Bridgewater)
    #25006
* worker:
  * Expose workers by default and remove `--experimental-worker` flag
    (Anna Henningsen) #25361

PR-URL: #25537
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
@joyeecheung joyeecheung removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 24, 2019
@joyeecheung

Copy link
Copy Markdown
Member

I am removing semver-major since this should not result in observable changes while fs is in the internal binding whitelist.

@BethGriggs

Copy link
Copy Markdown
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants