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: refactor structure of the os module #12654

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@jasnell
Copy link
Member

jasnell commented Apr 25, 2017

Refactoring the structure of the os module to use the more efficient module.exports = {} pattern.

Also adds simple Symbol.toPrimitive support for each of the os methods that return simple primitives.

Lastly, removes extraneous console output from the test-os.js

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

os

@jasnell jasnell changed the title Refactor os os: refactor structure of the os module Apr 25, 2017

@mscdex

This comment has been minimized.

Copy link
Contributor

mscdex commented Apr 25, 2017

What is the reason for the Symbol.toPrimitive changes?

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 25, 2017

Symbol.toPrimitive is an extremely simple convenience. For instance, os.endianness() always returns a constant string value. With the Symbol.toPrimitive, we can do:

// No parens at the end
`${os.endianess}`

instead of

`${os.endianness()}`

Likewise with the other functions that always return primitives.

It's extremely minor and quite cheap for us to support.

Another, example...

 `${os.platform}-${os.release}-${os.arch}-${os.hostname}`

Instead of

 `${os.platform()}-${os.release()}-${os.arch()}-${os.hostname()}`
test/parallel/test-os.js Outdated
/* eslint-disable no-restricted-properties */
[

[assert.equal, `${os.freemem}`, os.freemem()],

This comment has been minimized.

@Trott

Trott Apr 25, 2017

Member

Nit: Maybe better as

assert.strictEqual, `${os.freemem}`, `${os.freemem()}`]

Matter of style preference, I suppose.

This comment has been minimized.

@jasnell

jasnell Apr 25, 2017

Author Member

This was intentional, recognizing that the differences in the values should still be equal.

This comment has been minimized.

@Trott

Trott Apr 25, 2017

Member

This was intentional, recognizing that the differences in the values should still be equal.

You might be misunderstanding my suggestion. I know it's intentional. Explicitly converting the number to a string and using strictEqual() makes it more clear that it's intentional, at least in my opinion. OK by me, though, if you prefer the way it is.

This comment has been minimized.

@jasnell

jasnell Apr 25, 2017

Author Member

Sorry, I know what you meant, just wasn't being very clear. I'm using assert.equal() intentionally because the return value of the methods themselves are numbers and I want to ensure that the conversion to string after the Symbol.toPrimitive call returns an equal (but not strict equal) result. Either way I think gets the job done but to answer the question: yes, I prefer it this way :-)

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 27, 2017

@jasnell jasnell force-pushed the jasnell:refactor-os branch Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 27, 2017

@jasnell jasnell force-pushed the jasnell:refactor-os branch Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 27, 2017

Oops, I rebased while the CI was checking things out and goofed it up.
New New CI: https://ci.nodejs.org/job/node-test-pull-request/7717/

@jasnell jasnell force-pushed the jasnell:refactor-os branch Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 27, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 27, 2017

CI is green

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 29, 2017

@watilde
Copy link
Member

watilde left a comment

Looks similar to #12717. LGTM.

@aqrln

aqrln approved these changes Apr 29, 2017

Copy link
Member

aqrln left a comment

LGTM

@aqrln

This comment has been minimized.

Copy link
Member

aqrln commented Apr 29, 2017

Is it really semver-minor? Even though I don't think someone really used these functions in template literals as values themselves (as it doesn't make much sense), strictly speaking, the behavior has changed.

Before:

> `${os.endianness}`
'function () { return \'LE\'; }'

After:

> `${os.endianness}`
'LE'

It might be better to make this a major change just to be safe and strictly adhering to semver, especially given the fact that v8.0 is coming soon. It might be also worth to split this into two commits (just refactoring and Symbol.toPrimitive functionality) and have only one of them landed on v7.x.

However, a huge +1 to the change itself, about a week ago I really wished these functions were getters when I used them in template literals, while this PR solves it even more elegantly :)

@addaleax

This comment has been minimized.

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 29, 2017

I'm good with defensively labeling this as semver-major.

@jasnell jasnell added semver-major and removed semver-minor labels Apr 29, 2017

test/parallel/test-os.js Outdated
[assert.strictEqual, `${os.type}`, os.type()],
[assert.strictEqual, `${os.endianness}`, os.endianness()]
].forEach((set) => {
set[0](set[1], set[2], `${set[1]}, ${set[2]}`);

This comment has been minimized.

@addaleax

addaleax Apr 29, 2017

Member

I’m not actually sure whether the extra error message argument here is helpful, and maybe I would just inline assert.strictEqual here instead of passing it as part of set. (Just a suggestion, feel free to ignore me if you disagree)

This comment has been minimized.

@jasnell

jasnell Apr 29, 2017

Author Member

Now that they're all strictEquals.. +1 :-)

@jasnell jasnell force-pushed the jasnell:refactor-os branch Apr 29, 2017

jasnell added some commits Apr 25, 2017

os: refactor os structure, add Symbol.toPrimitive
Refactor the structure of the os module to use more efficient
module.exports = {} pattern.

Add Symbol.toPrimitive support to os methods that return simple
primitives. This is a minor tweak that makes using these slightly
more friendly when doing things like:

```js
var m = `${os.tmpdir}/foo`
```

@jasnell jasnell force-pushed the jasnell:refactor-os branch to 854d7b7 Apr 29, 2017

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Apr 29, 2017

jasnell added a commit that referenced this pull request May 1, 2017

os: refactor os structure, add Symbol.toPrimitive
Refactor the structure of the os module to use more efficient
module.exports = {} pattern.

Add Symbol.toPrimitive support to os methods that return simple
primitives. This is a minor tweak that makes using these slightly
more friendly when doing things like:

```js
var m = `${os.tmpdir}/foo`
```

PR-URL: #12654
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jasnell added a commit that referenced this pull request May 1, 2017

test: remove extra console output from test-os.js
PR-URL: #12654
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented May 1, 2017

Landed in 473572e and 23fc082

@jasnell jasnell closed this May 1, 2017

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

os: refactor os structure, add Symbol.toPrimitive
Refactor the structure of the os module to use more efficient
module.exports = {} pattern.

Add Symbol.toPrimitive support to os methods that return simple
primitives. This is a minor tweak that makes using these slightly
more friendly when doing things like:

```js
var m = `${os.tmpdir}/foo`
```

PR-URL: nodejs#12654
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

anchnk added a commit to anchnk/node that referenced this pull request May 6, 2017

test: remove extra console output from test-os.js
PR-URL: nodejs#12654
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

@jasnell jasnell referenced this pull request May 11, 2017

Closed

8.0.0 Release Proposal #12220

@Fishrock123

This comment has been minimized.

Copy link
Member

Fishrock123 commented Jun 1, 2017

As far as I can tell, adding Symbol.toPrimitive to bare Functions not a incompatible API change.

I'm bringing this up because I don't know how to possibly word this as a breaking change for the 8.0.0 breaking changes doc.

Reading function bodies in any real, dependant way is already not supported. (Obviously. This is like process.binding('natives'), and anyone possibly depending on this knows function bodies can and will change at any time.)

Adding the symbol does not alter any existing behavior of the actual documented functions, in any way (including typeof), or the module overall.

The behavior before, for coercion to "primitive", is effectively a common "misuse" error that under normal circumstances would be akin to any of our TypeErrors for invalid arguments. That is, you forgot to add the parens to call the function when you meant to.

This either: adds behavior that was "missing", or "fixes" previously "broken" behavior.

@jasnell, @addaleax, @aqrln Thoughts? Would I be ok to remove the label? (We can add dont-land-on- if ya'll want.)

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Jun 1, 2017

Would I be ok to remove the label?

I think so, yes. I don’t think this kind of change makes sense for LTS, so I’m adding the dont-land labels.

@aqrln

This comment has been minimized.

Copy link
Member

aqrln commented Jun 2, 2017

@Fishrock123 yes, I'm fine with it. Thanks for bringing this up.

@jasnell

This comment has been minimized.

Copy link
Member Author

jasnell commented Jun 2, 2017

In this particular case, dropping down to semver-minor is ok. We need to be careful about it in general with these kinds of changes, tho. Adding the don't land labels is ++

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