search

jprichardson / node-fs-extra

Node.js: extra methods for the fs object like copy(), remove(), mkdirs()
MIT License
9.44k stars 776 forks source link

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

Closed lamweili closed 2 years ago

lamweili commented 2 years ago

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 (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, 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 separately here: https://github.com/angular/angular/issues/45546

For your kind consideration, please.

RyanZim commented 2 years ago

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

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 (https://github.com/jprichardson/node-fs-extra/pull/936#issuecomment-1022044794) 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 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'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 (https://github.com/angular/angular/issues/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 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 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 commented 2 years ago

https://github.com/jprichardson/node-fs-extra/pull/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 commented 2 years ago

@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).

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

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

@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 commented 2 years ago

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

@JPeer264 I created a separate PR (https://github.com/jprichardson/node-fs-extra/pull/954) to migrate existing console.warn to process.emitWarning for existing codes.

lamweili commented 2 years ago

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

image

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

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

RyanZim commented 2 years ago

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

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

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

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

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

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

lamweili commented 2 years ago

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

lamweili commented 2 years ago

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