baudehlo / node-fs-ext

Extras missing from node's fs module
MIT License
101 stars 45 forks source link

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

Closed ikokostya closed 4 years ago

ikokostya commented 4 years ago

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:

https://github.com/baudehlo/node-fs-ext/blob/42b87130b3f01e78d5b5522f94ab6d955c57746d/fs-ext.js#L142

Other changes

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

Fixes https://github.com/baudehlo/node-fs-ext/issues/89 Fixes https://github.com/baudehlo/node-fs-ext/issues/81

ikokostya commented 4 years ago

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 commented 4 years ago

@baudehlo Done.

baudehlo commented 4 years ago

Thanks - I can't keep up with the way npm keeps changing how things are done these days - back when I wrote this you had to edit package.json manually.

On Thu, Nov 7, 2019 at 2:16 PM ikokostya notifications@github.com wrote:

@baudehlo https://github.com/baudehlo Done.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/baudehlo/node-fs-ext/pull/90?email_source=notifications&email_token=AAFBWY4W3ST2WHLZDEFSLGTQSRSRPA5CNFSM4JHT7T2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDNP7CY#issuecomment-551223179, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFBWYZGA4UWYUYKK5U4FC3QSRSRPANCNFSM4JHT7T2A .

baudehlo commented 4 years ago
$ 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 commented 4 years ago

Thanks for this! Real nice change.

ikokostya commented 4 years ago

Thanks for review!

baudehlo commented 4 years ago

I've also added you as a collaborator to the project.

On Thu, Nov 7, 2019 at 4:27 PM ikokostya notifications@github.com wrote:

Thanks for review!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/baudehlo/node-fs-ext/pull/90?email_source=notifications&email_token=AAFBWYZOSPBROXDXFBKBAJ3QSSB4DA5CNFSM4JHT7T2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDN36XY#issuecomment-551272287, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFBWY4S3MCK2ILGF2MQ3W3QSSB4DANCNFSM4JHT7T2A .