Skip to content

src: deprecate option variables in public API#22515

Closed
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:deprecate-option-vars
Closed

src: deprecate option variables in public API#22515
addaleax wants to merge 1 commit intonodejs:masterfrom
addaleax:deprecate-option-vars

Conversation

@addaleax
Copy link
Copy Markdown
Member

@addaleax addaleax commented Aug 24, 2018

These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@addaleax
src: deprecate option variables in public API
b78d57e 
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).
@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 24, 2018
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 24, 2018

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/667/pipeline

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Aug 24, 2018
@jdalton
Copy link
Copy Markdown
Member

jdalton commented Aug 25, 2018

@addaleax

These variables should never have been exposed as part of the
public API

Which API are they exposed in?

@devsnek
Copy link
Copy Markdown
Member

devsnek commented Aug 25, 2018 •
edited
Loading

@jdalton they can be accessed by embedders/addons via #include <node.h> and node::ssl_openssl_cert_store etc.

@tniessen
Copy link
Copy Markdown
Member

tniessen commented Aug 27, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16788/

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 27, 2018
@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Aug 28, 2018

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16822/

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Aug 31, 2018

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16911/

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Sep 2, 2018

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16937/

(Side note: It does feel a bit frustrating that our policies require wasting developer time and CI resources like this. This is the third unnecessary resume CI on this PR.)

@tniessen
Copy link
Copy Markdown
Member

tniessen commented Sep 2, 2018 •
edited by addaleax
Loading

Resumed in https://ci.nodejs.org/job/node-test-pull-request/16944/ (:heavy_check_mark:)

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Sep 2, 2018 •
edited
Loading

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/16945/

(edit: sorry, didn’t see @tniessen’s comment here.)

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Sep 2, 2018

Tobias’ CI is green, yay!

Landed in f911e09

@addaleax addaleax closed this Sep 2, 2018
@addaleax addaleax deleted the deprecate-option-vars branch September 2, 2018 15:39
addaleax added a commit that referenced this pull request Sep 2, 2018
@addaleax
src: deprecate option variables in public API
f911e09 
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).

PR-URL: #22515
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this pull request Sep 2, 2018
@addaleax @targos
src: deprecate option variables in public API
e92155a 
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).

PR-URL: #22515
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos
Copy link
Copy Markdown
Member

targos commented Sep 2, 2018

Is there a way to avoid our own use of these variables to trigger the warning?

@addaleax
Copy link
Copy Markdown
Member Author

addaleax commented Sep 2, 2018

@targos Good point! Yes: #22666

targos pushed a commit that referenced this pull request Sep 3, 2018
@addaleax @targos
src: deprecate option variables in public API
bac4c41 
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).

PR-URL: #22515
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@addaleax @targos
src: deprecate option variables in public API
da1f78c 
These variables should never have been exposed as part of the
public API, and certainly not as variables. Using CLI options
parser is the right thing to do here, at least until we expose
some part of the options parser API publicly (which should be
possible to do now).

PR-URL: #22515
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos added a commit that referenced this pull request Sep 6, 2018
@targos
2018-09-06, Version 10.10.0 (Current)
814a0c0 
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716
targos added a commit that referenced this pull request Sep 6, 2018
@targos
2018-09-06, Version 10.10.0 (Current)
cdb3598 
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716
targos added a commit that referenced this pull request Sep 6, 2018
@targos
2018-09-06, Version 10.10.0 (Current)
ed976d7 
Notable changes:

* child_process:
  * `TypedArray` and `DataView` values are now accepted as input by
    `execFileSync` and `spawnSync`. #22409
* coverage:
  * Native V8 code coverage information can now be output to disk by setting the
    environment variable `NODE_V8_COVERAGE` to a directory. #22527
* deps:
  * The bundled npm was upgraded to version 6.4.1. #22591
    * Changelogs:
      [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0)
      [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0)
      [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0)
      [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1)
* fs:
  * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`,
    `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and
    `DataView` objects. #22150
  * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and
    `fs.readdirSync`. If set to true, the methods return an array of directory
    entries. These are objects that can be used to determine the type of each
    entry and filter them based on that without calling `fs.stat`. #22020
* http2:
  * The `http2` module is no longer experimental. #22466
* os:
  * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to
    manipulate the scheduling priority of processes. #22407
* process:
  * Added `process.allowedNodeEnvironmentFlags`. This object can be used to
    programmatically validate and list flags that are allowed in the
    `NODE_OPTIONS` environment variable. #19335
* src:
  * Deprecated option variables in public C++ API. #22515
  * Refactored options parsing. #22392
* vm:
  * Added `vm.compileFunction`, a method to create new JavaScript functions from
    a source body, with options similar to those of the other `vm` methods. #21571
* Added new collaborators:
  * [lundibundi](https://github.com/lundibundi) - Denys Otrishko

PR-URL: #22716
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

+2 more reviewers

@refack refack refack approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.