fs: consider NaN/Infinity in toUnixTimestamp by yorkie · Pull Request #2387 · nodejs/node

yorkie · 2015-08-15T16:56:58Z

We might should follow the rule of linux:

If times is NULL, then the access and modification times of the file are set to the current time.

ref: http://linux.die.net/man/2/utime

mscdex · 2015-08-15T17:00:00Z

Why not replace both checks with !isFinite()?

yorkie · 2015-08-15T17:00:47Z

Oh, I absolutely this function, let me use and test it, thanks for the tip

yorkie · 2015-08-15T17:04:06Z

Fixed, thank you Brian

mscdex · 2015-08-15T17:05:00Z

I meant replace both conditions, like simply:

if(!isFinite(time)){

since !isFinite(NaN) === true

yorkie · 2015-08-15T17:08:57Z

Fixed, thank you

thefourtheye · 2015-08-15T17:57:43Z

What exactly to do here?

Sorry, it should be "done"

yorkie · 2015-08-16T02:33:15Z

Corrected, thanks @thefourtheye

sindresorhus · 2015-08-16T09:25:14Z

Should use Number.isFinite() instead of isFinite():
In comparison to the global isFinite() function, this method doesn't forcibly convert the parameter to a number. This means only values of the type number, that are also finite, return true.
Doesn't matter here, but good for consistency.

yorkie · 2015-08-16T11:47:35Z

After reading some docs about the difference at MDN and ECMA spec, I changed it to use the Number.isFinite is better than that global function. Thanks @sindresorhus

yorkie · 2015-08-19T13:16:27Z

Any feedback from core team?

trevnorris · 2015-08-21T23:03:32Z

LGTM

thefourtheye · 2015-08-22T09:47:06Z

Hmmm.

what about the negative numbers?
Also, we should mention this behaviour in the docs as well, I think.

yorkie · 2015-08-22T12:54:18Z

what about the negative numbers?

I have no idea about the negative, because the linux manual didn't mention this, so I have the following proposals:

Treat it as NULL and set the date to Date.now()
Date.now() + time means minus the absolute value
Do nothing

Also, we should mention this behaviour in the docs as well, I think.

I will document this either after getting the answer of the above from @thefourtheye or other Node.js team member. Thank you :)

yorkie · 2015-08-26T05:07:11Z

any words? @thefourtheye

yorkie · 2015-08-26T15:06:46Z

How is this pr going? @thefourtheye did ignore my questions and proposals again, I don't know what happened :(

trevnorris · 2015-08-26T16:00:19Z

@yorkie Might be outside the scope of this PR, but is there utility in accepting strings? The check would basically be:

if(typeoftime==='string'&&+time==time){return+time;}

I'm cool w/ the change if CI is happy.

yorkie · 2015-08-27T09:16:53Z

Hey @trevnorris I Added your suggestion at this PR, but I still care about strict mode and source code confusion in using the operator ==.

Mainly, the unit test is pass on my side :)

yorkie · 2015-08-27T09:18:33Z

Added few document as well, but I have not yet got any response about minus value, so I will just keep the current logic.

thefourtheye · 2015-08-27T10:37:42Z

@yorkie I am really sorry about the delay. I finally got some time to test this. The value of -1 for atime and mtime seems to reset the times to UNIX Epoch time, in my Ubuntu box.

thefourtheye · 2015-08-27T10:38:52Z

yorkie · 2015-08-27T13:42:27Z

It's fine @thefourtheye

reset the times to UNIX Epoch time

Do you have some document about this or someone from nodejs team tell me what should I do, or I think set to Unix Epoch is not still the best solution.

trevnorris · 2015-08-27T14:57:00Z

@yorkie Thanks. Only reason I suggest using +val == val is because it's the only single operation to check for both stringified hex and floating point values.

thefourtheye · 2015-08-27T14:59:58Z

@yorkie I searched for documentations but I couldn't find any. I found that by running

constfs=require('fs');console.error(fs.statSync('Test.py'));fs.utimesSync('Test.py',-1,-1);console.error(fs.statSync('Test.py'));

yorkie · 2015-08-27T15:11:44Z

Thanks for your work @thefourtheye, I will follow your running result at first :)

yorkie · 2015-08-27T15:26:25Z

Updated, now the minus number and other Date.now() has been set to corresponding Epoch timestamp, @thefourtheye could you please review again, thank you :)

thefourtheye · 2015-08-27T15:27:46Z

@yorkie Ah, lets get some confirmation about the negative values. @bnoordhuis any idea?

yorkie · 2015-08-27T16:20:37Z

Of course, and I will document something after that :)

yorkie · 2015-08-28T17:27:58Z

Ping @bnoordhuis

yorkie · 2015-09-14T13:34:56Z

@orangemocha so the thing that I should or can do is reword the title and push again?

yorkie · 2015-09-14T13:36:10Z

Linter would break at this line? @Fishrock123

orangemocha · 2015-09-14T13:42:48Z

so the thing that I should or can do is reword the title and push again?

@yorke : yes!

yorkie · 2015-09-14T13:45:01Z

Yes, sir. I will fix the line 131 as well :)

orangemocha · 2015-09-14T13:46:41Z

@yorkie : or, if you added those tests in the first place, you could also do an interactive rebase and change your commits to simply not add them.

yorkie · 2015-09-14T13:51:32Z

Ok, I did squash all commits then could someone run the CI again, thanks for your helps :)

orangemocha · 2015-09-14T15:00:30Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/298/

yorkie · 2015-09-14T15:33:29Z

The test seems to be suspended by ARM at https://ci.nodejs.org/job/node-test-commit-arm/634/console, anything that I can help to fix?

trevnorris · 2015-09-14T22:39:38Z

@yorkie Just took a really long time to finish. Everything looks green now.

PR-URL: #2387 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris · 2015-09-14T22:56:16Z

Landed on 1f842c2 with column change in doc.

Fishrock123 · 2015-09-15T01:48:00Z

To clarify, this is indeed a breaking change?

yorkie · 2015-09-15T04:32:37Z

Thanks @trevnorris

To clarify, this is indeed a breaking change?

Shouldn't, I guess few modules use Nan or Infinity internally, and in my opinion this seems to be a bug fix?

trevnorris · 2015-09-15T17:23:14Z

@Fishrock123 I'd say this is a bug-fix. This simply makes previously undefined behavior defined.

PR-URL: #2387 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) nodejs#2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) nodejs#2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) nodejs#2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) nodejs#2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) nodejs#2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) nodejs#2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) nodejs#2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) nodejs#2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (nodejs#2507). Refs: nodejs#2844 PR-URL: nodejs#2889

Notable changes: * buffer: - Buffers are now created in JavaScript, rather than C++. This increases the speed of buffer creation (Trevor Norris) #2866. - `Buffer#slice()` now uses `Uint8Array#subarray()` internally, increasing `slice()` performance (Karl Skomski) #2777. * fs: - `fs.utimes()` now properly converts numeric strings, `NaN`, and `Infinity` (Yazhong Liu) #2387. - `fs.WriteStream` now implements `_writev`, allowing for super-fast bulk writes (Ron Korving) #2167. * http: Fixed an issue with certain `write()` sizes causing errors when using `http.request()` (Fedor Indutny) #2824. * npm: Upgrade to version 2.14.3, see https://github.com/npm/npm/releases/tag/v2.14.3 for more details (Kat Marchán) #2822. * src: V8 cpu profiling no longer erroneously shows idle time (Oleksandr Chekhovskyi) #2324. * v8: Lateral upgrade to 4.5.103.33 from 4.5.103.30, contains minor fixes (Ali Ijaz Sheikh) #2870. - This fixes a previously known bug where some computed object shorthand properties did not work correctly (#2507). Refs: #2844 PR-URL: #2889

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Aug 15, 2015

mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 15, 2015

thefourtheye reviewed Aug 15, 2015
View reviewed changes

sindresorhus reviewed Aug 16, 2015
View reviewed changes

thefourtheye reviewed Aug 27, 2015
View reviewed changes

Comment threaddoc/api/fs.markdown Outdated
thefourtheyeAug 27, 2015
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If

yorkie reviewed Sep 14, 2015
View reviewed changes

Comment threadtest/parallel/test-fs-utimes.js Outdated
yorkieSep 14, 2015
Copy link
Copy Markdown
ContributorAuthor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linter would break at this line? @Fishrock123

fs: consider NaN/Infinity in toUnixTimestamp
4442dfe

yorkie force-pushed the fix/fs branch from 5839b8d to 4442dfeCompare September 14, 2015 13:50

trevnorris pushed a commit that referenced this pull request Sep 14, 2015
fs: consider NaN/Infinity in toUnixTimestamp
1f842c2
PR-URL: #2387 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

trevnorris closed this Sep 14, 2015

yorkie deleted the fix/fs branch September 15, 2015 04:27

rvagg mentioned this pull request Sep 15, 2015
Release proposal: v3.3.1 #2698
Merged

Fishrock123 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 15, 2015

Fishrock123 pushed a commit that referenced this pull request Sep 15, 2015
fs: consider NaN/Infinity in toUnixTimestamp
6108ea9
PR-URL: #2387 Reviewed-By: Trevor Norris <trev.norris@gmail.com>

Fishrock123 mentioned this pull request Sep 15, 2015
Release proposal: v4.1.0 #2844
Closed
7 tasks

rvagg mentioned this pull request Sep 22, 2015
Release proposal: v4.1.1 #2995
Merged

TimothyGu mentioned this pull request Jul 4, 2017
regression: fs.utimes/futimes for dates before unix epoch #14017
Closed

Uh oh!

Uh oh!

Conversation

yorkie commented Aug 15, 2015

Uh oh!

mscdex commented Aug 15, 2015

Uh oh!

yorkie commented Aug 15, 2015

Uh oh!

yorkie commented Aug 15, 2015

Uh oh!

mscdex commented Aug 15, 2015

Uh oh!

yorkie commented Aug 15, 2015

Uh oh!

thefourtheyeAug 15, 2015

Choose a reason for hiding this comment

Uh oh!

yorkieAug 16, 2015

Choose a reason for hiding this comment

Uh oh!

yorkie commented Aug 16, 2015

Uh oh!

sindresorhusAug 16, 2015

Choose a reason for hiding this comment

Uh oh!

yorkie commented Aug 16, 2015

Uh oh!

yorkie commented Aug 19, 2015

Uh oh!

trevnorris commented Aug 21, 2015

Uh oh!

thefourtheye commented Aug 22, 2015

Uh oh!

yorkie commented Aug 22, 2015

Uh oh!

yorkie commented Aug 26, 2015

Uh oh!

yorkie commented Aug 26, 2015

Uh oh!

trevnorris commented Aug 26, 2015

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

thefourtheye commented Aug 27, 2015

Uh oh!

thefourtheyeAug 27, 2015

Choose a reason for hiding this comment

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

trevnorris commented Aug 27, 2015

Uh oh!

thefourtheye commented Aug 27, 2015

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

thefourtheye commented Aug 27, 2015

Uh oh!

yorkie commented Aug 27, 2015

Uh oh!

yorkie commented Aug 28, 2015

Uh oh!

yorkie commented Sep 14, 2015

Uh oh!

yorkieSep 14, 2015

Choose a reason for hiding this comment

Uh oh!

orangemocha commented Sep 14, 2015

Uh oh!

yorkie commented Sep 14, 2015

Uh oh!

orangemocha commented Sep 14, 2015

Uh oh!

yorkie commented Sep 14, 2015

Uh oh!