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.

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

os: make EOL configurable and readonly #14622

Closed
wants to merge 1 commit into from

Conversation

@XadillaX
Copy link
Member

@XadillaX XadillaX commented Aug 4, 2017

Refs: #14619

Checklist
  • make -j4 test (UNIX)
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

os


const eol = common.isWindows ? '\r\n' : '\n';

assert.strictEqual(eol, os.EOL);

This comment has been minimized.

@richardlau

richardlau Aug 4, 2017
Member

Assertion parameters should be actual, expected so I think these should be the other way around.

Copy link
Member

@tniessen tniessen left a comment

LGTM with @richardlau's comment addressed.


assert.strictEqual(eol, os.EOL);

common.expectsError(function() {

This comment has been minimized.

@tniessen

tniessen Aug 4, 2017
Member

I know that we didn't reach consensus on this, but I personally prefer arrow functions in our tests to make them a little more readable.

This comment has been minimized.

@XadillaX

XadillaX Aug 4, 2017
Author Member

IMHO I'd like to use anonymous function.

Refs: #14496 (comment)

Even though there was no result about that discussion.

@cjihrig
cjihrig approved these changes Aug 4, 2017
@lpinca
lpinca approved these changes Aug 4, 2017
@refack
Copy link
Member

@refack refack commented Aug 4, 2017

Optional

@XadillaX we could take this one step further. Move EOL to node_contants.c defined with NODE_DEFINE_CONSTANT. Then the os.EOL property would be just a getter {get: () => constants.EOL} (maybe even deprecate it, now or later).

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 4, 2017

@ChALkeR ... any way we can get an estimate on any breakage this may cause? (if any)

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Aug 4, 2017

@refack Those constants are all numbers, used as flags to be passed between C++ and JS. I don't see a reason to introduce an inconsistency here.

lib/os.js Outdated
},

EOL: {
configurable: false,

This comment has been minimized.

@TimothyGu

TimothyGu Aug 4, 2017
Member

On a second thought, maybe making it configurable would be better? This way people who really need to change it for testing can do so, but requiring them to explicitly state their intentions. I'm okay either way though.

This comment has been minimized.

@XadillaX

XadillaX Aug 6, 2017
Author Member

Okay.

Copy link
Member

@addaleax addaleax left a comment

I think @TimothyGu’s comment is a blocker.

(Also, I’m kind of missing the motivation here… people don’t accidentally override os.EOL, do they?)

@richardlau
Copy link
Member

@richardlau richardlau commented Aug 4, 2017

(Also, I’m kind of missing the motivation here… people don’t accidentally override os.EOL, do they?)

From #14619:

os.EOL is specified as a constant but its value is getting updated

So an alternative is to update the docs.

@tniessen
Copy link
Member

@tniessen tniessen commented Aug 4, 2017

people don’t accidentally override os.EOL, do they?

@addaleax In my opinion, this should throw when in strict mode:

if(os.EOL = 'foo') {
  console.log('Nope');
}

I am ±0 about making it configurable, but it should be a constant by default.

Copy link
Member

@TimothyGu TimothyGu left a comment

Still LGTM, just even more so!

@aqrln
aqrln approved these changes Aug 6, 2017
@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Aug 7, 2017

@addaleax How's the code now?

@addaleax addaleax dismissed their stale review Aug 7, 2017

outdated

@addaleax
Copy link
Member

@addaleax addaleax commented Aug 7, 2017

I’ve removed my Changes Requested label, but as I mentioned I don’t quite see the point here.

Maybe it’s a miscommunication about what “constant” means in this context; if documentation for JS code says that foo.bar is a constant, I personally would expect that to mean that the value is not changed by whatever code provides that value, i.e. in this specific case that Node doesn’t change that value while a process is running.

@refack
Copy link
Member

@refack refack commented Aug 7, 2017

@jasnell did a grep on Gzemnid dump from 2017-02-23, no assignment to EOL found.
@XadillaX added a test case for configurability in 3e31ac4, feel free to push out or edit.

@jasnell jasnell added the ctc-review label Aug 8, 2017
@jasnell
Copy link
Member

@jasnell jasnell commented Aug 8, 2017

Marking this one ctc-review. @nodejs/ctc ... please weigh in

@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Aug 11, 2017

@jasnell, @joyeecheung just reviewed

Copy link
Member

@jasnell jasnell left a comment

LGTM with good CI and CITGM run.

@rvagg
rvagg approved these changes Aug 14, 2017
Copy link
Member

@Fishrock123 Fishrock123 left a comment

¯\_(ツ)_/¯

@not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Aug 14, 2017

-0. In general I'm not sure it's a good idea to make properties like this non-configurable, because it removes an escape hatch. I can imagine a hypothetical test helper to verify an that application works on multiple platforms, which could rely on mutating os.EOL. We don't have any evidence that the current behavior has caused problems in practice, so changing it seems like it's a solution without a problem.

If we're convinced that we want to make the property non-writable to avoid accidental mutation, could we keep the property configurable? That way it would still be possible to overwrite the property as an escape hatch, but users would be unlikely to mutate it by mistake.

edit: I just saw that it is configurable the current version of the PR. The title of the PR still says it's non-configurable.

@refack
refack approved these changes Aug 14, 2017
Copy link
Member

@refack refack left a comment

LGTM with { configurable: true }

@refack
Copy link
Member

@refack refack commented Aug 14, 2017

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/951/

I count 4 ✔️s from CTC members, so IMHO this is ready to land (pending CitGM)

@aqrln aqrln changed the title os: make EOL non-configurable and readonly os: make EOL configurable and readonly Aug 15, 2017
@mcollina
Copy link
Member

@mcollina mcollina commented Aug 16, 2017

before landing, can this get a rebase? We are getting some failures for body-parser that are already fixed on master.

Copy link
Member

@mcollina mcollina left a comment

LGTM if CITGM is ok

@XadillaX XadillaX force-pushed the XadillaX:feature/os-eol branch Aug 17, 2017
@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Aug 17, 2017

@mcollina rebased.

@not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Aug 17, 2017

Reminder to whoever lands this: the commit message should be updated to correct "non-configurable" to "configurable".

@XadillaX XadillaX changed the title os: make EOL configurable and readonly os: make EOL non-configurable and readonly Aug 17, 2017
@XadillaX XadillaX changed the title os: make EOL non-configurable and readonly os: make EOL configurable and readonly Aug 17, 2017
Refs: #14619
@XadillaX XadillaX force-pushed the XadillaX:feature/os-eol branch to 6f5087d Aug 17, 2017
@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Aug 17, 2017

@not-an-aardvark Thanks and I've rebased the message.

@mcollina
Copy link
Member

@mcollina mcollina commented Aug 17, 2017

I think CITGM is green, this can be landed.

@aqrln
Copy link
Member

@aqrln aqrln commented Aug 17, 2017

@XadillaX regarding the commit message, I think the patch "Fixes:", not just "Refs:" the issue, doesn't it?

@XadillaX
Copy link
Member Author

@XadillaX XadillaX commented Aug 21, 2017

@aqrln Right, I think who to land this may help me to modify the commit message.

@refack
Copy link
Member

@refack refack commented Aug 21, 2017

Landed in f6caeb9

@refack refack closed this Aug 21, 2017
refack added a commit that referenced this pull request Aug 21, 2017
PR-URL: #14622
Fixes: #14619
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott Trott removed the ctc-review label Aug 21, 2017
jasnell added a commit that referenced this pull request Oct 17, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this pull request Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(SEMVER-MAJOR)** [#14414](#14414)

* **Errors**
  * Multiple built in modules have been migrated to use static error codes

* **Domains**
  * The long deprecated `.dispose()` method has been removed
    [[`602fd36d95`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.