Skip to content

Don't override fs module, export constants as object#90

Merged
baudehlo merged 3 commits intobaudehlo:masterfrom
ikokostya:dont-override-fs
Nov 7, 2019
Merged

Don't override fs module, export constants as object#90
baudehlo merged 3 commits intobaudehlo:masterfrom
ikokostya:dont-override-fs

Conversation

@ikokostya
Copy link
Copy Markdown
Collaborator

@ikokostya ikokostya commented Oct 31, 2019

This PR removes override of fs and constants core Node.js modules. Instead all functions and constants should be imported explicitly:

const {flock, constants} = require('fs-ext');
// or
const fsExt = require('fs-ext');
// fsExt.flock
// fsExt.constants

In C++ binding all constants are placed to constants object instead of target object, e.g. on my machine:

> require('./build/Release/fs-ext')
{ constants:
   { SEEK_SET: 0,
     SEEK_CUR: 1,
     SEEK_END: 2,
     LOCK_SH: 1,
     LOCK_EX: 2,
     LOCK_NB: 4,
     LOCK_UN: 8,
     F_GETFD: 1,
     F_SETFD: 2,
     FD_CLOEXEC: 1,
     F_RDLCK: 0,
     F_WRLCK: 1,
     F_UNLCK: 2,
     F_SETLK: 6,
     F_GETLK: 5,
     F_SETLKW: 7 },
  seek: [Function],
  fcntl: [Function],
  flock: [Function],
  statVFS: [Function] }

This helps to export constants on JavaScript side without pattern matching:

if (/^[A-Z_]+$/.test(key) && !constants.hasOwnProperty(key)) {

Other changes

  • Use fsExt module in tests instead binding directly.
  • Update examples in README.md to use modern JavaScript (destructuring, arrow functions), because minimum supported version is Node.js 8.

Please note that it's breaking change. Therefore, major version should be updated.

Fixes #89
Fixes #81

{
"name": "fs-ext",
"version": "1.2.2",
"version": "1.3.0",
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't touch this file. I think it was synchronized with package.json during execution of npm install command.

@ikokostya ikokostya mentioned this pull request Nov 4, 2019
Copy link
Copy Markdown
Owner

@baudehlo baudehlo left a comment

Choose a reason for hiding this comment

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

Looks good, but needs a major version bump (package.json), and something in the README about the change.

@ikokostya
Copy link
Copy Markdown
Collaborator Author

ikokostya commented Nov 7, 2019

Looks good, but needs a major version bump (package.json)

OK. Usually maintainer updates version of the package before publishing it to npm repository:

npm version major
npm pubish

But I can update version manually in package.json. It's not a problem.

and something in the README about the change.

I think that best place for this is new CHANGELOG.md file.

@ikokostya
Copy link
Copy Markdown
Collaborator Author

@baudehlo Done.

@baudehlo
Copy link
Copy Markdown
Owner

baudehlo commented Nov 7, 2019 via email

@baudehlo baudehlo merged commit 3125353 into baudehlo:master Nov 7, 2019
@baudehlo
Copy link
Copy Markdown
Owner

baudehlo commented Nov 7, 2019

$ npm publish
npm notice 
npm notice 📦  fs-ext@2.0.0
npm notice === Tarball Contents === 
npm notice 986B   package.json                 
npm notice 1.6kB  .eslintrc                    
npm notice 404B   .travis.yml                  
npm notice 242B   appveyor.yml                 
npm notice 219B   binding.gyp                  
npm notice 1.3kB  example.js                   
npm notice 17.1kB fs-ext.cc                    
npm notice 3.4kB  fs-ext.js                    
npm notice 1.0kB  LICENSE.txt                  
npm notice 3.2kB  README.md                    
npm notice 310B   run_tests.js                 
npm notice 316B   wscript                      
npm notice 7.7kB  tests/test-fs-fcntl.js       
npm notice 5.7kB  tests/test-fs-flock_stress.js
npm notice 10.7kB tests/test-fs-flock.js       
npm notice 9.1kB  tests/test-fs-seek_stress.js 
npm notice 14.8kB tests/test-fs-seek.js        
npm notice 267B   tests/test-fs-statvfs.js     
npm notice === Tarball Details === 
npm notice name:          fs-ext                                  
npm notice version:       2.0.0                                   
npm notice package size:  14.6 kB                                 
npm notice unpacked size: 78.4 kB                                 
npm notice shasum:        e58168fcc37506a9358e0928f4aae60912af7caa
npm notice integrity:     sha512-aK8NlpSO5LUdS[...]u1aGS8deJwzBQ==
npm notice total files:   18                                      
npm notice 
+ fs-ext@2.0.0

@baudehlo
Copy link
Copy Markdown
Owner

baudehlo commented Nov 7, 2019

Thanks for this! Real nice change.

@ikokostya
Copy link
Copy Markdown
Collaborator Author

Thanks for review!

@ikokostya ikokostya deleted the dont-override-fs branch November 7, 2019 21:27
@baudehlo
Copy link
Copy Markdown
Owner

baudehlo commented Nov 7, 2019 via email

sheetalkamat pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Nov 19, 2019
* [fs-ext] Update definition to v2

* Remove definitions for utime(), utimeSync().
* Add constants definition.
* Add new values of `cmd` parameter to fcntl(), fcntlSync().
* Add overrides for more accurate types of `flags` parameter in flock(), flockSync().
* Add overrides for more accurate types of `cmd` parameter in fcntl(), fcntlSync().
* Add statVFS() defintion.
* Rewrite tests.

Refs baudehlo/node-fs-ext#90

* Use NodeJS.ErrnoException instead of Error

See https://github.com/baudehlo/node-fs-ext/blob/3125353321c30bd35fc38e40ce76ffe3872e215b/fs-ext.cc#L366
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.

Don't override fs module F_WRLCK not added to constants in Node.js 11

2 participants