test: add test for Module._stat - #44713
Conversation
Sorry, something went wrong.
This comment was marked as outdated.
aduh95
left a comment
There was a problem hiding this comment.
Maybe add mustCalls to ensure it’s indeed being called?
Sorry, something went wrong.
|
@aduh95 aren't mustCalls for making sure that a callback passed to a function gets called? This one's totally synchronous, that's why I tested this completely based on the change in return values. |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Wait, that's not a use case we want to support, mutating fs should not have any effect on Node.js internals, we should fix that.
Sorry, something went wrong.
There was a problem hiding this comment.
This is actually how VFSs are implemented in the ecosystem (pkg, electron, etc.) currently and fixing that would break a lot of packages and I believe the intention behind exposing Module._stat is to allow this? I don't think there is any other use case behind Module._stat or Module._readPackage. cc @arcanis
FWIW, we are also trying to find better ways of doing this without monkey-patching in nodejs/single-executable#37.
Sorry, something went wrong.
There was a problem hiding this comment.
It shows that we probably also need Module._realPath and Module._readFileSync – or rather, that we need the loader hook API to stabilize. Anyway, I don't know if we want this is our tests, I think we want to break this at some point.
Sorry, something went wrong.
There was a problem hiding this comment.
@aduh95 if we start exposing Module._* functions for these, we would have to do so for a lot more functions. These are the ones that Electron overrides - https://github.com/electron/electron/blob/eebf34cc6c4691e2ddca9b5a0a97566aeabd9072/lib/asar/fs-wrapper.ts#L236-L854 (quite a lot!) and there are probably additional ones in yarn's fslib implementation - https://github.com/yarnpkg/berry/tree/76ccb18b3b8cc81e28dbef5f3f867395aa31d5fb/packages/yarnpkg-fslib/sources/patchFs.
Sorry, something went wrong.
There was a problem hiding this comment.
Fwiw I personally have an expectation that Node.js should abide to its own fs API (which is part of why _stat and _readPackage were so problematic, being the two places not doing so purely for optimization purposes).
It's probably never been discussed formally before though, and perhaps doing so would be a good thing (if only to get this use case formally recognized, supported, and covered by tests).
Sorry, something went wrong.
There was a problem hiding this comment.
I agree that the lack of consistency is quite bad. IMHO Node.js internals should not be affected by user-land actions, however I could see that we still want to support the use case of alternative fs implementation, which could be supplied by e.g. a CLI flag and would affect the whole process, not just the few files where we forgot to use destructuring.
Sorry, something went wrong.
|
cc @nodejs/modules |
Sorry, something went wrong.
It’s pretty close. Once #44710 and #43772 land and we allow some time for baking, that’s all that’s on our list before declaring the API stable: https://github.com/nodejs/loaders#status |
Sorry, something went wrong.
|
To be clear, the PR that exposed Module._stat has already landed and it has been released to v18 and v16, I'm not sure why a test for such a thing wouldn't be accepted. Do y'all have suggestions on what needs to be changed in the test for it to be accepted? I don't think it's a good idea to have untested features laying around. If y'all are not happy that the feature exists, would y'all be acceptive of a PR that reverts #44537? |
Sorry, something went wrong.
There was a problem hiding this comment.
Fwiw I don't have real approval rights in this repo, but as far as I can tell the test looks fine to me 🙂
(As for reverting, I strongly object - I made the original PR for a reason, it shouldn't be reverted)
Sorry, something went wrong.
Where's the suggestion for reverting? Even if we eventually revert (not that I'm suggesting we do) it would be preferable to revert both the feature and its test together, I think, so we have the history. |
Sorry, something went wrong.
I posted the comment about reverting in #44713 (comment) in case we are not comfortable with exposing Module._stat.
Yes but for that we would need to land the test first. If you take a look at the contents of #44537 you would see that it landed without any tests, which is why I thought of sending a PR to add some. |
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Virtual CommonJS files rooted at process.execPath inherit the output directory's type=module package unless the runner hides it alongside Chromium's root package. Ref: nodejs/node#44713 Co-Authored-By: GitHub Copilot <copilot@github.com>
Module._stat landed in #44537 without a test, so this change adds one.
Signed-off-by: Darshan Sen raisinten@gmail.com