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

src: deprecate option variables in public API #22515

Closed
wants to merge 1 commit into from
Closed

src: deprecate option variables in public API #22515

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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
Collaborator

@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
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
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
Member

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
Member Author

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

@addaleax
Copy link
Member Author

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

@addaleax
Copy link
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
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
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
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 <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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 <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos
Copy link
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
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 <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@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 <me@gus.host>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
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

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@tniessen tniessen tniessen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@devsnek devsnek devsnek approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr approved these changes

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.