Skip to content

feat: support bigint option in fs.stat, fs.lstat, and fs.fstat - #325

Merged
tschaub merged 7 commits into
mainfrom
stats-bigint
May 9, 2021
Merged

feat: support bigint option in fs.stat, fs.lstat, and fs.fstat#325
tschaub merged 7 commits into
mainfrom
stats-bigint

Conversation

@3cp

@3cp 3cp commented May 8, 2021

Copy link
Copy Markdown
Collaborator

closes #321

BREAKING CHANGE: drop support of Nodejs before v12.0.0

@3cp

3cp commented May 8, 2021

Copy link
Copy Markdown
Collaborator Author

fillStatsArray needs to be updated to support bigint too.

@3cp
3cp force-pushed the stats-bigint branch from 7483a4d to 4836e6b Compare May 8, 2021 09:08
@3cp
3cp marked this pull request as draft May 8, 2021 10:10
@3cp 3cp changed the title feat: support bigint option in fs.stat, fs.lstat, and fs.fstat WIP feat: support bigint option in fs.stat, fs.lstat, and fs.fstat May 8, 2021
@3cp

3cp commented May 8, 2021

Copy link
Copy Markdown
Collaborator Author

There is one more stat array in fs binding: bigintStatValues needs to be implemented.

> process.binding('fs')
BaseObject {
  statValues: Float64Array(36) [
      16777220,     33188,          1,       501,
            20,         0,       4096, 102210022,
             0,         0, 1620469235, 811292225,
    1620469235, 811292225, 1620469235, 811292225,
    1620469235, 811292225,          0,         0,
             0,         0,          0,         0,
             0,         0,          0,         0,
             0,         0,          0,         0,
             0,         0,          0,         0
  ],
  bigintStatValues: BigUint64Array(36) [
      16777220n,      33188n,          1n,
           501n,         20n,          0n,
          4096n,   98442580n,       1270n,
             8n, 1615088646n,  360473114n,
    1615088646n,  243729709n, 1615088646n,
     243729709n, 1615082739n,  729366298n,
             0n,          0n,          0n,
             0n,          0n,          0n,
             0n,          0n,          0n,
             0n,          0n,          0n,
             0n,          0n,          0n,
             0n,          0n,          0n
  ],

@3cp

3cp commented May 8, 2021

Copy link
Copy Markdown
Collaborator Author

Since nodejs v10.0.0, the internal return of binding.stat/lstat/fstat are now Float64Array or BigUint64Array. The old nodejs returns a Stats object with properties like stats.mode.

I am trying to make it work for both old and new nodejs. The implementation might work (I have not tested enough), but there are tons of test code needs two copies (one for old, and one for new).

Since nodejs v10.0.0 is already EOL, @tschaub should we consider for a major version upgrade to target only nodejs v12+? The benefits are:

  1. can remove lots of tricky code.
  2. also upgrade all dev deps to latest.

@tschaub

tschaub commented May 8, 2021

Copy link
Copy Markdown
Owner

Since nodejs v10.0.0 is already EOL, @tschaub should we consider for a major version upgrade to target only nodejs v12+?

Sounds like a good plan.

@3cp
3cp force-pushed the stats-bigint branch from 0aad557 to a9072c8 Compare May 9, 2021 02:16
closes #321

BREAKING CHANGE: drop support of Nodejs before v12.0.0
@3cp
3cp force-pushed the stats-bigint branch from a9072c8 to ec82d7c Compare May 9, 2021 02:45
@3cp

3cp commented May 9, 2021

Copy link
Copy Markdown
Collaborator Author

The added code are for test.
The code in lib folder is 100 LOC less.

huocp@chunpengs-mbp ~/g/mock-fs (stats-bigint)> cloc lib/
      12 text files.
      12 unique files.                              
       0 files ignored.

github.com/AlDanial/cloc v 1.88  T=0.03 s (423.4 files/s, 100451.6 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                      12            282            836           1729
-------------------------------------------------------------------------------
SUM:                            12            282            836           1729
-------------------------------------------------------------------------------

huocp@chunpengs-mbp ~/g/mock-fs (stats-bigint)> git checkout main
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
huocp@chunpengs-mbp ~/g/mock-fs (main)> cloc lib/
      12 text files.
      12 unique files.                              
       0 files ignored.

github.com/AlDanial/cloc v 1.88  T=0.03 s (410.1 files/s, 103698.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
JavaScript                      12            295            908           1831
-------------------------------------------------------------------------------
SUM:                            12            295            908           1831
-------------------------------------------------------------------------------

@3cp

3cp commented May 9, 2021

Copy link
Copy Markdown
Collaborator Author

I will get a win10 box to test the NaN uid/gid thing.

@3cp 3cp changed the title WIP feat: support bigint option in fs.stat, fs.lstat, and fs.fstat feat: support bigint option in fs.stat, fs.lstat, and fs.fstat May 9, 2021
@3cp
3cp marked this pull request as ready for review May 9, 2021 10:30
@3cp
3cp requested a review from tschaub May 9, 2021 10:30
@3cp

3cp commented May 9, 2021

Copy link
Copy Markdown
Collaborator Author

GitHub actions still wants to see nodejs 6/8/10 results. I don't know how to remove them in the checks.

Windows now uses 0 for uid and gid, not NaN anymore (this is probably a change in nodejs v10.0.0 when stats is a Float64Array internally).

@3cp

3cp commented May 9, 2021

Copy link
Copy Markdown
Collaborator Author

@tschaub after merging this one, you might want to update README to clarify nodejs support in mock-fs v4 and v5.
Also I think it's a good idea to highlight the missing support of fs.opendir #319 which will unlikely be fixed in future.

@tschaub tschaub left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very impressive! Thanks for all the effort on this, @3cp.

@tschaub
tschaub merged commit 093b5a6 into main May 9, 2021
@3cp

3cp commented May 9, 2021

Copy link
Copy Markdown
Collaborator Author

@tschaub I will do a small cleanup of uid/gid for Windows soon. It's unnecessary to use NaN internally in our code.

@3cp
3cp deleted the stats-bigint branch May 9, 2021 20:15
Comment thread .eslintrc
"env": {
"es2020": true
}
}

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this has the effect of overriding instead of extending the existing config in package.json. I've opened #327 to move this to package.json and conform with the existing rules.

@tschaub

tschaub commented May 10, 2021

Copy link
Copy Markdown
Owner

mock-fs@5.0.0-beta.1 includes this change.

@3cp

3cp commented May 23, 2021

Copy link
Copy Markdown
Collaborator Author

@tschaub can you release v5.0.0?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with realpath on symlink with Node 15.14.0

2 participants