Revert check for fs.realpath.native (#887)

[lamweili]

#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.

node-fs-extra/lib/fs/index.js

Line 57 in 7edcb16

exports.realpath.native = u(fs.realpath.native)

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)

(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. Incompatibility with fs ".native" functions heimdalljs/heimdalljs#151
2. memfs breaks with fs-extra^10.0.0 streamich/memfs#803
3. TypeError: Cannot read property 'name' of undefined using log4js (^6.4.0) with angular in ssr mode log4js-node/log4js-node#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, 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, log4js-node/log4js-node#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 separately here: angular/angular#45546

---

For your kind consideration, please.

[RyanZim]

I can see both sides of the argument here. On one hand, defensive coding is good. On the other hand, monkey-patching core modules is a terrible practice, and if you do it anyway, but then fail to do it correctly, I find it hard to be sympathetic.

[lamweili]

Agreed but sometimes, these monkey patches come from other embedded dependencies (such as the Angular case), which puts users in a difficult situation between the dependency and fs-extra. The async-listeners (#936 (comment)) that monkey-patches the core modules (yes, agreed that it is a terrible practice and still failed to do it correctly) was discovered after 5-levels of dependencies chains that's a little beyond the users' control. 🤯

---

I'm coming as a contributor of log4js which uses streamroller that uses fs-extra@^10.0.1.

I'm posting this on behalf of my user @myuniverse8 for his issue log4js-node/log4js-node#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's initialisation). We do not not even use fs-extra.realpath.native.

There's nothing much I can do for him, except to plead my case to both Angular (angular/angular#45546) and to you, @RyanZim.

---

I believe many out there are affected as well, when they use dependencies 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 log4js-node/log4js-node#1225 by @myuniverse8
async-listener #936 (comment) by @matanarbel
homebridge #926 (comment) by @hepcat72
homebridge #926 (comment) by @roman-teamsystems
memfs streamich/memfs#803 by @Phillip9587
nexe #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 fs-extra.realpath.native API can be optional if not used in runtime (pre-#887) so it should not be an initialisation showstopper from #887; it seems like fs-extra issue when it's not!

---

#887 isn't documented in the changelog (should be in 10.0.0). So perhaps we can revert out of goodwill as a defensive practice? It will improve fs-extra's compatibility.

Apart from reverting, we can add in warnings to flag out if fs.realpath.native is undefined, for people to pay attention to fix their dependencies, and so fs-extra.realpath.native will not available. Let me know and I'll add a commit for it for this PR.

The pros outweighs the cons
I hope that changes your mind. 🙏

[RyanZim]

#887 isn't documented in the changelog (should be in 10.0.0).

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

[lamweili]

@RyanZim Sounds great, I shall start on the works for the warning flags in parallel to expedite 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 for our case?
• If we are moving to process.emitWarning, should I also migrate the codes below (probably in a separate PR) for consistency?

---

node-fs-extra/lib/copy/copy.js

Lines 24 to 28 in e0d298d

	// Warn about using preserveTimestamps on 32-bit node
	if (opts.preserveTimestamps && process.arch === 'ia32') {
	console.warn(`fs-extra: Using the preserveTimestamps option in 32-bit node is not recommended;\n
	see https://github.com/jprichardson/node-fs-extra/issues/269`)
	}

node-fs-extra/lib/copy/copy-sync.js

Lines 18 to 22 in e0d298d

	// Warn about using preserveTimestamps on 32-bit node
	if (opts.preserveTimestamps && process.arch === 'ia32') {
	console.warn(`fs-extra: Using the preserveTimestamps option in 32-bit node is not recommended;\n
	see https://github.com/jprichardson/node-fs-extra/issues/269`)
	}

[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. After some thoughts the deprecated warning does not make sense in this case.

[lamweili]

@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?

[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?

[lamweili]

@JPeer264 I created a separate PR (#954) to migrate existing console.warn to process.emitWarning for existing codes.

[lamweili]

For the current issue, the warnings when fs.realpath.native is undefined looks like:

I have also added the following if fs.realpath.native = undefined for some backward compatibility support:

• test case to verify that the warnings are emitted
• fallback for fs-extra.realpath.native to use u(fs.realpath) (they have the same method signature) for older NodeJS (warned anyway)
• test case to verify that the fallback (fs.realpath) is working correctly

I hope this looks good to be merged, together with #954.

[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.

[lamweili]

Strange, the newly added tests ran successfully when I npm test locally. 😩

I am going to set it to draft while I try to fix the mocha tests for the CI issues. It might be due to require.cache. I will set to ready for review once completed.

---

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.

That would require .npmrc to be configured with engine-strict=true if I remember correctly. IMHO, it would be more holistic should people somehow skips the engines enforcement at package.json, so that its clear if its NodeJS versioning issue or from random dependencies altering fs.

The added fallback mitigates cases where fs-extra.realpath.native is actually used in code, as it implicitly falls back to u(fs.realpath) as a safety net so it's more forgiving towards runtime error. Should the behaviour be undesirable (it shouldn't be since it is asserted in the test case), we already have an initialisation warning about the fallback.

Any thoughts?

[lamweili]

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.

[lamweili]

@RyanZim After some thoughts, you are right. Good call!👍
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 does not export.

node-fs-extra/lib/fs/index.js

Lines 102 to 107 in e0d298d

	// fs.writev only available in Node v12.9.0+
	if (typeof fs.writev === 'function') {
	// Function signature is
	// s.writev(fd, buffers[, position], callback)
	// We need to handle the optional arg, so we use ...args
	exports.writev = function (fd, buffers, ...args) {

I shall remove the fallback and amend the warning message.

At the very least, now fs-extra does not fail on initialisation due to the fault of others' poor monkey-patches on fs.

[lamweili]

@RyanZim @JPeer264
Thanks for bearing with my verbosity. Learnt a great deal from you guys.
It should look good now?

[RyanZim]

LGTM; will give others a chance to review before merging.

[lamweili]

Thank you, @RyanZim @JPeer264 for your patience and review.
Sounds good to go!