os: make EOL configurable and readonly by XadillaX · Pull Request #14622 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX)
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

os

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

LGTM with @richardlau's comment addressed.

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.

IMHO I'd like to use anonymous function.

Refs: #14496 (comment)

Even though there was no result about that discussion.

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).

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

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

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.

Okay.

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?)

(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.

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.

Still LGTM, just even more so!

@addaleax How's the code now?

outdated

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.

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

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

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

@jasnell, @joyeecheung just reviewed

LGTM with good CI and CITGM run.

¯\_(ツ)_/¯

-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.

LGTM with { configurable: true }

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)

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

LGTM if CITGM is ok

@mcollina rebased.

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/957/

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

Refs: nodejs#14619

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

I think CITGM is green, this can be landed.

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

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

Pre-land CI: https://ci.nodejs.org/job/node-test-commit/11929/

Landed in f6caeb9

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>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(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`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

nodejs-github-bot added the os Issues and PRs related to the os subsystem. label Aug 4, 2017

XadillaX added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 4, 2017

vsemozhetbyt approved these changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 4, 2017

View reviewed changes

richardlau reviewed Aug 4, 2017

View reviewed changes

tniessen approved these changes Aug 4, 2017

View reviewed changes

cjihrig approved these changes Aug 4, 2017

View reviewed changes

lpinca approved these changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 4, 2017

View reviewed changes

addaleax previously requested changes Aug 4, 2017

View reviewed changes

TimothyGu approved these changes Aug 6, 2017

View reviewed changes

aqrln approved these changes Aug 6, 2017

View reviewed changes

jasnell added the ctc-review label Aug 8, 2017

joyeecheung approved these changes Aug 10, 2017

View reviewed changes

jasnell approved these changes Aug 11, 2017

View reviewed changes

Trott mentioned this pull request Aug 13, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-08-16 nodejs/CTC#162

Closed

rvagg approved these changes Aug 14, 2017

View reviewed changes

Fishrock123 reviewed Aug 14, 2017

View reviewed changes

refack approved these changes Aug 14, 2017

View reviewed changes

aqrln changed the title os: make EOL non-configurable and readonly os: make EOL configurable and readonly Aug 15, 2017

mcollina approved these changes Aug 16, 2017

View reviewed changes

XadillaX force-pushed the feature/os-eol branch from 3e31ac4 to 042c259 Compare August 17, 2017 03:53

XadillaX changed the title os: make EOL configurable and readonly os: make EOL non-configurable and readonly Aug 17, 2017

XadillaX changed the title os: make EOL non-configurable and readonly os: make EOL configurable and readonly Aug 17, 2017

os: make EOL configurable and readonly

6f5087d

Refs: nodejs#14619

XadillaX force-pushed the feature/os-eol branch from 042c259 to 6f5087d Compare August 17, 2017 09:49

refack closed this Aug 21, 2017

Trott removed the ctc-review label Aug 21, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged


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

		assert.strictEqual(eol, os.EOL);


		assert.strictEqual(eol, os.EOL);

		common.expectsError(function() {

+                },
+                EOL: {
+                  configurable: false,

os: make EOL configurable and readonly by XadillaX · Pull Request #14622 · nodejs/node · GitHub

Uh oh!

Uh oh!

os: make EOL configurable and readonly - #14622

os: make EOL configurable and readonly#14622XadillaX wants to merge 1 commit intonodejs:masternodejs/node:masterfrom XadillaX:feature/os-eolXadillaX/node:feature/os-eolCopy head branch name to clipboard

Conversation

XadillaX commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

richardlau Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

tniessen left a comment

Choose a reason for hiding this comment

Uh oh!

tniessen Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

XadillaX Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Optional

Uh oh!

jasnell commented Aug 4, 2017

Uh oh!

TimothyGu commented Aug 4, 2017

Uh oh!

TimothyGu Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

XadillaX Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

richardlau commented Aug 4, 2017

Uh oh!

tniessen commented Aug 4, 2017

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

XadillaX commented Aug 7, 2017

Uh oh!

addaleax commented Aug 7, 2017

Uh oh!

refack commented Aug 7, 2017

Uh oh!

refack commented Aug 7, 2017

Uh oh!

jasnell commented Aug 8, 2017

Uh oh!

XadillaX commented Aug 11, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

not-an-aardvark commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Aug 14, 2017

Uh oh!

mcollina commented Aug 16, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

XadillaX commented Aug 17, 2017

Uh oh!

os: make EOL configurable and readonly#14622
XadillaX wants to merge 1 commit into
nodejs:masterfrom
XadillaX:feature/os-eol

XadillaX commented Aug 4, 2017 •
edited
Loading

refack commented Aug 4, 2017 •
edited
Loading

not-an-aardvark commented Aug 14, 2017 •
edited
Loading