Repository: jprichardson/node-fs-extra
Pull request #953 (2022-04-06 13:40:31)

Revert check for fs.realpath.native (#887)
#887 causes a few regressions, not because of `fs-extra`, but due to other dependencies who are using `fs-extra`.

**The general gist is that others' improper monkey-patches of `fs` should not impact the import and initialisation of `fs-extra`.**

---

When `fs.realpath.native` is `undefined` caused by the improper handling by other packages, it inevitably affected the import and initialisation of `fs-extra` as the `universalify` would not be able to parse this line properly.
https://github.com/jprichardson/node-fs-extra/blob/7edcb16a06e041826af3303f961866bf3b243dae/lib/fs/index.js#L57

**It results in `fs-extra` throwing errors on import, even if the `fs-extra.realpath.native` API is not being used during runtime.**

And the stacktrace shows:
```
    TypeError: Cannot read property 'name' of undefined

      at Object.<anonymous>.exports.fromCallback (node_modules/universalify/index.js:15:26)
      at Object.<anonymous> (node_modules/fs-extra/lib/fs/index.js:57:27)
      at Object.<anonymous> (node_modules/fs-extra/lib/index.js:5:6)
```
![image](https://user-images.githubusercontent.com/29530649/161987896-73679265-dcc8-4f2c-a068-9482ef535e01.png)
_(src: https://github.com/RyanZim/universalify/blob/a853a4aedc63c69fcdc62b77643d75b0d162a098/index.js#L15)_

---

While it is up to the responsibility of the underlying packages to fix their root cause of poor monkey-patches to `fs`, **it might be wise for `fs-extra` to keep the check for `fs.realpath.native` during initialisation so that `fs-extra` does not throw errors on import.**

To name a few:
1. https://github.com/heimdalljs/heimdalljs/issues/151
2. https://github.com/streamich/memfs/issues/803
3. https://github.com/log4js-node/log4js-node/issues/1225

To summarise from the 3 listed issues above, it does seem like most dependencies did not factor in `fs` third-level functions. From the [NodeJS fs API documentation](https://nodejs.org/docs/latest-v14.x/api/fs.html), most are second-level functions (i.e. fs.xxx). But there are 2 exceptions since NodeJS 9.2.0 that are in third-level (i.e. fs.xxx.yyy), which are `fs.realpath.native` and `fs.realpathSync.native`. So if any the packages did some wrapping around, which usually naively only handles direct members (second-level), the third-level functions breaks and becomes `undefined`. 

Even in Angular (traced from the 3rd issue above, https://github.com/log4js-node/log4js-node/issues/1225), they, too, did not factor in the 2 special functions.
https://github.com/angular/angular/blob/1c11a5715576632a4fb7170202395cf95dfbce09/packages/zone.js/lib/node/fs.ts#L20-L26

I have filed an issue to [zone.js/node](https://github.com/angular/angular/) separately here: https://github.com/angular/angular/issues/45546

---

**For your kind consideration, please.**

peteriman: Agreed but sometimes, these monkey patches come from other dependencies (such as the Angular case), which puts users in a difficult situation between the dependency and `fs-extra`.

I'm coming as a contributor of [log4js](https://github.com/log4js-node/log4js-node) which uses [streamroller](https://github.com/log4js-node/streamroller) that uses `fs-extra@^10.0.1`. 

I'm posting this on behalf of my user @myuniverse8 for his issue https://github.com/log4js-node/log4js-node/issues/1225. He uses `log4js` (which implicitly uses `fs-extra`) on Angular SSR. Yet, due to Angular's patching, `fs.realpath.native` broke and became `undefined`, resulting in errors when he attempts imports `log4js`. 
 
There's nothing much I can do for him, except to plead my case to both Angular (https://github.com/angular/angular/issues/45546) and to you, @RyanZim. 

I plead for his case, because I believe many out there are affected as well, not limited to `Angular/zone.js/node` or `memfs` when they use together with `fs-extra` through direct or indirect dependency. 

#887 isn't documented in the changelog (since 10.0.0). So perhaps it won't be that of an issue to revert out of goodwill as a defensive practise? It will grant some time for dependencies to patch. 

It can be re-implemented perhaps in the next major release, as it would be possibly breaking change that is construed by those poor monkey-patches that a user uses together with `fs-extra`.

I hope that changes your mind. 🙏
peteriman: Agreed but sometimes, these monkey patches come from other dependencies (such as the Angular case), which puts users in a difficult situation between the dependency and `fs-extra`.

I'm coming as a contributor of [log4js](https://github.com/log4js-node/log4js-node) which uses [streamroller](https://github.com/log4js-node/streamroller) that uses `fs-extra@^10.0.1`. 

I'm posting this on behalf of my user @myuniverse8 for his issue https://github.com/log4js-node/log4js-node/issues/1225. He uses `log4js` (which implicitly uses `fs-extra`) on Angular SSR. Yet, due to Angular's patching, `fs.realpath.native` broke and became `undefined`, resulting in errors when he attempts to import `log4js` (as `universalify` throws error when called by `fs-extra` initialisation) 
 
There's nothing much I can do for him, except to plead my case to both Angular (https://github.com/angular/angular/issues/45546) and to you, @RyanZim. 

I believe many out there are affected as well, not limited to `Angular/zone.js/node` or `memfs`, when they use poor monkey-patches together with `fs-extra` (through direct or indirect dependency). 

#887 isn't documented in the changelog (since 10.0.0). So perhaps we can revert out of goodwill as a defensive practise? It will grant some time for poorly implemented money-patches dependencies to fix. 

After a granted leeway in timeline, it can be re-implemented in the next major release, as it would be possibly breaking change that is construed by those poor monkey-patches (still didn't managed to fix) that a user uses together with `fs-extra`.

I hope that changes your mind. 🙏
peteriman: Agreed but sometimes, these monkey patches come from other dependencies (such as the Angular case), which puts users in a difficult situation between the dependency and `fs-extra`.

---

I'm coming as a contributor of [log4js](https://github.com/log4js-node/log4js-node) which uses [streamroller](https://github.com/log4js-node/streamroller) that uses `fs-extra@^10.0.1`. 

I'm posting this on behalf of my user @myuniverse8 for his issue https://github.com/log4js-node/log4js-node/issues/1225. He uses `log4js` (which implicitly uses `fs-extra`) on Angular SSR. Yet, due to Angular's patching, `fs.realpath.native` broke and became `undefined`, resulting in errors when he attempts to import `log4js` (`universalify` throws an error when called by `fs-extra` initialisation) 
 
There's nothing much I can do for him, except to plead my case to both Angular (https://github.com/angular/angular/issues/45546) and to you, @RyanZim. 

---

I believe many out there are affected as well, when they use dependencies (not limited to `Angular/zone.js/node` or `memfs`) that poorly monkey-patches `fs`,  together with `fs-extra` as a direct or indirect dependency. 

Some, including from `Angular/zone.js/node` and `memfs`, are:

> `Angular/zone.js/node` https://github.com/log4js-node/log4js-node/issues/1225 by @myuniverse8
> `async-listener` https://github.com/jprichardson/node-fs-extra/pull/936#issuecomment-1022044794 by @matanarbel
> `homebridge` https://github.com/jprichardson/node-fs-extra/issues/926#issuecomment-945014637 by @hepcat72 
> `homebridge` https://github.com/jprichardson/node-fs-extra/issues/926#issuecomment-961816850 by @roman-teamsystems 
> `memfs` https://github.com/streamich/memfs/issues/803 by @Phillip9587
> `nexe` https://github.com/jprichardson/node-fs-extra/issues/909 by @donthijackthegit 

And it can be seen immensely difficult to trace to the underlying culprit, up to 5 levels of dependencies for `async-listener`. I'm sure there are more to the list from the collateral damage. In addition, the feature of `fs.realpath.native` can be optional (pre-#887) for `fs-extra` so it should not be a showstopper from #887. 

---

#887 isn't documented in the [changelog (should be in 10.0.0)](https://github.com/jprichardson/node-fs-extra/blob/69eb8964626871000de3a95548cd36035b5c4b27/CHANGELOG.md). So perhaps we can revert out of goodwill as a defensive practice? It will improve `fs-extra`'s compatibility and at the same time grant some time for poorly implemented monkey-patched dependencies to fix should this change be implemented at a later date. 

In the meantime, apart from reverting, we can add in warnings to flag out if `fs.realpath.native` is `undefined` for people to pay attention. Let me know and I'll add a commit for it for this PR. 

After a granted leeway in the timeline (after all there is no immediate need for this change), it can be re-implemented in the next major release, as it would be possibly breaking change that is construed by those poor monkey-patches (still didn't manage to fix) that a user uses together with `fs-extra`.

The pros outweighs the cons
I hope that changes your mind. 🙏
RyanZim: > https://github.com/jprichardson/node-fs-extra/pull/887 isn't documented in the [changelog (should be in 10.0.0)](https://github.com/jprichardson/node-fs-extra/blob/69eb8964626871000de3a95548cd36035b5c4b27/CHANGELOG.md).

Yeah, it was just implicitly assumed in the Node version requirement bump.

I do sort of like the idea of a warning flag, but would like to get some input from the other maintainers here. @JPeer264 @manidlou @jprichardson 
peteriman: @RyanZim Sounds great, I shall start on the works for the warning flags in parallel while waiting for the inputs.

I found some existing codes using `console.warn` (refer to end of comment).

- Do we want to follow with that, or should we use [process.emitWarning](https://nodejs.org/api/process.html#processemitwarningwarning-type-code-ctor) for our case?
- Or should I also migrate them to [process.emitWarning](https://nodejs.org/api/process.html#processemitwarningwarning-type-code-ctor) for consistency?

---

https://github.com/jprichardson/node-fs-extra/blob/e0d298d297ce4998b02dd86748f68b302d590bff/lib/copy/copy.js#L24-L28
https://github.com/jprichardson/node-fs-extra/blob/e0d298d297ce4998b02dd86748f68b302d590bff/lib/copy/copy-sync.js#L18-L22



JPeer264: I like the idea about the warning flag as well. On top, we can also add the JSDoc's `@deprecated` (https://usejsdoc.org/tags-deprecated.html) flag, so IDE's, which supports this, display it as deprecated and people can react earlier.
peteriman: @JPeer264 What are your thoughts on the warning flag implementation?

Should we continue to use `console.warn` (referencing existing codes), or should we migrate all to [process.emitWarning](https://nodejs.org/api/process.html#processemitwarningwarning-type-code-ctor)? 
JPeer264: Yes `process.emitWarning` sounds also like a great option to add (just my opinion on it). However, a separate PR would be nice if you want to go the extra mile as we squash our PRs. @manidlou @RyanZim @jprichardson any objections?
JPeer264: Yes `process.emitWarning` sounds also like a great option to add (just my opinion on it). However, a separate PR would be nice if you want to go the extra mile as we squash our PRs. @manidlou @RyanZim @jprichardson any objections?
peteriman: @JPeer264 I shall create a separate PR to migrate existing `console.warn` to `process.emitWarning` for existing codes (for better consistency). 
peteriman: For the current issue, the warnings for `fs.realpath.native` be `undefined` looks like:

![image](https://user-images.githubusercontent.com/29530649/162501353-c20f6ebe-d355-4bf3-95aa-eef1cb3c555e.png)

I have also added the following if `fs.realpath.native = undefined` for some backward compatibility support
  - [x] test case to verify that the warnings are emitted
  - [x] fallback to use `fs.realpath` (they have the same method signature) for older NodeJS
  - [x] test case to verify that the fallback (`fs.realpath`) is working correctly

RyanZim: I like the warning stuff so far; however, there's no reason to do a fallback or Node version checking. Our `engines` field already takes care of old Node versions at the package manager level.
peteriman: Strange, the tests ran successfully when I `npm test`.
Let me try to use `done` for the mocha tests to see if it fixes the CI issues. 
peteriman: I hope I have fixed the CI issues resulting from the added tests. 
I am unable to replicate them on my machines now.

Would need a maintainer's approval to run the workflow to determine if it truly resolves.
RyanZim: npm will still warn about incompatible engines on install, even without `engine-strict`.

Fallback is unnecessary; if you actually used `fs.realpath.native` in your code, you'd have a problem _without_ `fs-extra`; it's not within scope for `fs-extra` to fix other people's failed monkey-patches.
peteriman: @RyanZim After some thoughts, you are right. 👍
It's not within scope to fix others' failed monkey-patch and should not be exported by `fs-extra` if not available.

It follows the consistent principle of `fs-extra` (such as `fs.writev`); if not available/problematic, `fs-extra` do not export.
https://github.com/jprichardson/node-fs-extra/blob/e0d298d297ce4998b02dd86748f68b302d590bff/lib/fs/index.js#L102-L107

I shall remove the fallback and amend the warning message.
peteriman: @RyanZim @JPeer264 
It should look good now? 
peteriman: @RyanZim @JPeer264
Thanks for bearing with my verbosity.
It should look good now?


peteriman: @RyanZim, changes completed.
peteriman: @RyanZim, changes completed in https://github.com/jprichardson/node-fs-extra/commit/38b85a08dc3b36db94bf93a889cc72fd8251d99f.
peteriman: @RyanZim, changes completed in https://github.com/jprichardson/node-fs-extra/commit/38b85a08dc3b36db94bf93a889cc72fd8251d99f.
peteriman: Thank you, @RyanZim @JPeer264 for your patience and review.
Sounds good to go! 

Patch:
diff --git a/lib/fs/index.js b/lib/fs/index.js
index e460b74..7b025e2 100644
--- a/lib/fs/index.js
+++ b/lib/fs/index.js
@@ -54,7 +54,6 @@ Object.assign(exports, fs)
 api.forEach(method => {
   exports[method] = u(fs[method])
 })
-exports.realpath.native = u(fs.realpath.native)
 
 // We differ from mz/fs in that we still ship the old, broken, fs.exists()
 // since we are a drop-in replacement for the native module
@@ -117,3 +116,13 @@ if (typeof fs.writev === 'function') {
     })
   }
 }
+
+// fs.realpath.native sometimes not available if fs is monkey-patched
+if (typeof fs.realpath.native === 'function') {
+  exports.realpath.native = u(fs.realpath.native)
+} else {
+  process.emitWarning(
+    'fs.realpath.native is not a function. Is fs being monkey-patched?',
+    'Warning', 'fs-extra-WARN0003'
+  )
+}


Test patch:
diff --git a/lib/fs/__tests__/realpath.test.js b/lib/fs/__tests__/realpath.test.js
index 9306859..b03249b 100644
--- a/lib/fs/__tests__/realpath.test.js
+++ b/lib/fs/__tests__/realpath.test.js
@@ -1,11 +1,67 @@
 'use strict'
 
-const fse = require('../..')
+const fs = require('fs')
+const path = require('path')
 const assert = require('assert')
 
 /* eslint-env mocha */
 
+describe('realpath.native does not exist', () => {
+  let warning
+  const warningListener = error => {
+    if (error.name === 'Warning') {
+      if (error.code.startsWith('fs-extra-WARN0003')) {
+        warning = error
+      }
+    }
+  }
+
+  const realpathNativeBackup = fs.realpath.native
+  const clearFseCache = () => {
+    const fsePath = path.dirname(require.resolve('../../..'))
+    for (const entry in require.cache) {
+      if (entry.startsWith(fsePath)) {
+        delete require.cache[entry]
+      }
+    }
+  }
+
+  before(() => {
+    process.on('warning', warningListener)
+
+    // clear existing require.cache
+    clearFseCache()
+
+    // simulate fs monkey-patch
+    delete fs.realpath.native
+  })
+
+  after(() => {
+    process.off('warning', warningListener)
+
+    // clear stubbed require.cache
+    clearFseCache()
+
+    // reinstate fs.realpath.native
+    fs.realpath.native = realpathNativeBackup
+  })
+
+  it('fse should not export realpath.native', done => {
+    const fse = require('../..')
+
+    // next event loop to allow event emitter/listener to happen
+    setImmediate(() => {
+      assert(warning, 'fs-extra-WARN0003 should be emitted')
+      done()
+    })
+
+    assert(!fse.realpath.native)
+  })
+})
+
 describe('realpath.native', () => {
+  const fse = require('../..')
+
   it('works with callbacks', () => {
     fse.realpath.native(__dirname, (err, path) => {
       assert.ifError(err)