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

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

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@shisama
Copy link
Contributor

shisama commented Aug 23, 2018

Refs: #22160

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

This comment has been minimized.

Copy link
Member

addaleax commented Aug 23, 2018

I’d prefer not to rush this one, given the history we have with fs bindings… Can we maybe land it early after the Node 11 semver-major cut?

@addaleax

This comment has been minimized.

@jasnell jasnell referenced this pull request Aug 23, 2018

Closed

Tracking Issue: process.binding to internalBinding #22160

32 of 33 tasks complete
@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Aug 23, 2018

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

@jdalton

This comment has been minimized.

Copy link
Member

jdalton commented Aug 23, 2018

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

@shisama shisama force-pushed the shisama:internal-binding-fs branch 3 times, most recently from 121225f to 69f7b28 Aug 24, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Aug 25, 2018

Thank you for your reviews. I fixed them.

@shisama shisama force-pushed the shisama:internal-binding-fs branch 2 times, most recently from 3dd21e1 to e367e77 Aug 27, 2018

@BridgeAR
Copy link
Member

BridgeAR left a comment

LGTM for version 12.

@BridgeAR BridgeAR added the blocked label Aug 31, 2018

@jdalton

This comment has been minimized.

Copy link
Member

jdalton commented Sep 4, 2018

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

This comment has been minimized.

Copy link
Member

jasnell commented Sep 4, 2018

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

@targos

This comment has been minimized.

Copy link
Member

targos commented Sep 4, 2018

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

This comment has been minimized.

Copy link
Member

jasnell commented Sep 4, 2018

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

@jasnell

This comment has been minimized.

Copy link
Member

jasnell commented Sep 4, 2018

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 Sep 4, 2018

This is failing in CI

@jasnell
Copy link
Member

jasnell left a comment

There are relevant CITGM failures that need to be investigated.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Sep 5, 2018

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 shisama:internal-binding-fs branch 3 times, most recently from 8855aa7 to af2a68b Sep 13, 2018

@shisama shisama force-pushed the shisama:internal-binding-fs branch from af2a68b to b7a5a87 Oct 17, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Oct 18, 2018

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

@joyeecheung

This comment has been minimized.

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

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 12, 2018

@joyeecheung Thank you. I squashed them.

@shisama shisama force-pushed the shisama:internal-binding-fs branch from 7b2d4a8 to 79834c9 Nov 13, 2018

@shisama shisama force-pushed the shisama:internal-binding-fs branch from 79834c9 to df729bc Nov 13, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 13, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 13, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 13, 2018

@BridgeAR Hi, is the label blocked still needed?

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Nov 13, 2018

I don't think so.

@BridgeAR BridgeAR removed the blocked label Nov 13, 2018

@shisama

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 16, 2018

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

shisama added a commit that referenced this pull request Nov 16, 2018

fs: make process.binding('fs') internal
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

This comment has been minimized.

Copy link
Contributor Author

shisama commented Nov 16, 2018

Landed in 1e23e3c

@shisama shisama closed this Nov 16, 2018

joyeecheung added a commit to joyeecheung/node that referenced this pull request Jan 11, 2019

fs: make process.binding('fs') internal
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 added a commit that referenced this pull request Jan 14, 2019

fs: make process.binding('fs') internal
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 referenced this pull request Jan 16, 2019

Merged

v11.7.0 proposal #25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 16, 2019

fs: make process.binding('fs') internal
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

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

PR-URL: nodejs#25537

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 17, 2019

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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

2019-01-17, Version 11.7.0 (Current), @BridgeAR
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 referenced this pull request Jan 24, 2019

Merged

v11.8.0 proposal #25687

@joyeecheung

This comment has been minimized.

Copy link
Member

joyeecheung commented Jan 24, 2019

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

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