Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from Apr 16, 2022
Merged

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

merged 6 commits into from Apr 16, 2022

Conversation

lamweili
Copy link
Contributor

@lamweili lamweili commented Apr 6, 2022

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

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)

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. 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
Copy link
Collaborator

RyanZim commented Apr 6, 2022

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
Copy link
Contributor Author

lamweili commented Apr 6, 2022

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
Copy link
Collaborator

RyanZim commented Apr 7, 2022

#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
Copy link
Contributor Author

lamweili commented Apr 8, 2022

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

// 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`)
}

// 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
Copy link
Collaborator

JPeer264 commented Apr 8, 2022

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
Copy link
Contributor Author

lamweili commented Apr 8, 2022

@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
Copy link
Collaborator

JPeer264 commented Apr 8, 2022

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
Copy link
Contributor Author

lamweili commented Apr 8, 2022

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

@lamweili lamweili marked this pull request as draft April 8, 2022 17:21
@lamweili lamweili marked this pull request as ready for review April 8, 2022 18:18
@lamweili
Copy link
Contributor Author

lamweili commented Apr 8, 2022

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:

  • 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
Copy link
Collaborator

RyanZim commented Apr 8, 2022

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
Copy link
Contributor Author

lamweili commented Apr 8, 2022

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 lamweili marked this pull request as draft April 9, 2022 00:48
@lamweili lamweili closed this Apr 9, 2022
@lamweili lamweili reopened this Apr 9, 2022
@lamweili lamweili marked this pull request as ready for review April 9, 2022 04:32
@lamweili
Copy link
Contributor Author

lamweili commented Apr 9, 2022

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.

Signed-off-by: Lam Wei Li <peteriman@mail.com>
lib/fs/index.js Outdated Show resolved Hide resolved
@RyanZim
Copy link
Collaborator

RyanZim commented Apr 9, 2022

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
Copy link
Contributor Author

lamweili commented Apr 9, 2022

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

// 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
Copy link
Contributor Author

lamweili commented Apr 9, 2022

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

Signed-off-by: Lam Wei Li <peteriman@mail.com>
lib/fs/__tests__/realpath.test.js Outdated Show resolved Hide resolved
lib/fs/__tests__/realpath.test.js Outdated Show resolved Hide resolved
lib/fs/index.js Outdated Show resolved Hide resolved
…arning

Signed-off-by: Lam Wei Li <peteriman@mail.com>
lib/fs/__tests__/realpath.test.js Show resolved Hide resolved
lib/fs/index.js Outdated Show resolved Hide resolved
@lamweili lamweili requested a review from RyanZim April 11, 2022 15:05
lamweili and others added 2 commits April 11, 2022 23:09
Signed-off-by: Lam Wei Li <peteriman@mail.com>
Co-authored-by: Ryan Zimmerman <17342435+RyanZim@users.noreply.github.com>
Copy link
Collaborator

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

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

@lamweili
Copy link
Contributor Author

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

austinkelleher added a commit to JupiterOne/sdk that referenced this pull request Apr 16, 2022
The following packages have been upgraded:

- `@pollyjs/adapter-node-http`
- `@pollyjs/core`
- `@pollyjs/persister-fs`

The new Polly.JS packages ship with TypeScript definition files,
so the old `@type/pollyjs__` packages have been removed!

Additionally, `fs-extra` has been pinned to semver range `^9.0.0`
because there a change was released in `^10.0.0` that caused
a regression. See: jprichardson/node-fs-extra#953.
austinkelleher added a commit to JupiterOne/sdk that referenced this pull request Apr 16, 2022
The following packages have been upgraded:

- `@pollyjs/adapter-node-http`
- `@pollyjs/core`
- `@pollyjs/persister-fs`

The new Polly.JS packages ship with TypeScript definition files,
so the old `@type/pollyjs__` packages have been removed!

Additionally, `fs-extra` has been pinned to semver range `^9.0.0`
because there a change was released in `^10.0.0` that caused
a regression. See: jprichardson/node-fs-extra#953.
@RyanZim RyanZim merged commit 7bb0120 into jprichardson:master Apr 16, 2022
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.

None yet

3 participants