streamich / memfs

JavaScript file system utilities
http://streamich.github.io/memfs/
Apache License 2.0
1.76k stars 130 forks source link

Support win32 paths #327

Open heygrady opened 5 years ago

heygrady commented 5 years ago

https://github.com/webpack/webpack-dev-middleware/pull/366#issuecomment-462072465

memfs does not handle win32 paths starting with C:\ (unless you are actually on a windows machine). Probably not on your roadmap but it's a blocker for a personal pipe dream of replacing memory-fs with memfs.

const {vol} = require('memfs')
// treated as relative
vol.mkdirpSync("C:\\test")  // <-- creates a folder in my current working directory.
console.log(vol.toJSON())
// { '/Users/username/my-test/C:\\test': null }
const {vol} = require('memfs')
// treated as relative (and throws errors as expected)
// https://github.com/streamich/memfs/blob/master/docs/relative-paths.md
vol.mkdirSync("C:\\test"); // <-- throws Error: ENOENT: no such file or directory
vol.mkdirSync("C:\\"); // <-- throws Error: ENOENT: no such file or directory

By comparison, memory-fs treats paths starting with a / as probably posix and starting with [A-Z]: as probably win32. In the examples above, the presence of C: would be enough to convince memory-fs that it was an absolute win32 path.

memory-fs has no support for relative paths and throws if it encounters one. The relative path compromise that memfs makes is probably preferred as it's more similar to what fs does. But in the case of a win32 path prefix, memory-fs makes the more flexible choice.

FWIW, on my machine (a Mac), the following works exactly as memfs does. I don't know what it does on a Windows machine.

const fs = require('fs')
// treated as relative
fs.mkdirSync("C:\\test") // <-- creates a folder in my current working directory.
heygrady commented 5 years ago

I looked deeper. Seems like you handle win32 correctly but only when process.platform === 'win32'.

Is there a way to temporarily enable win32 mode? Would it be possible to inspect paths to see if there are absolute win32 paths? Or, can you recommend a way to write tests for win32 mode?

// it seems like this would _always_ truthy.
// on my mac, `pathModule === pathModule.posix` and `pathModule !== pathModule.win32`
// however, `!!pathModule.win32 === true`
if (pathModule.posix) {
  const { posix } = pathModule;
  sep = posix.sep;
  relative = posix.relative;
} else {
  // would this even happen? is `path.posix` simply not available on windows?
  sep = pathModule.sep;
  relative = pathModule.relative;
}

const isWin = process.platform === 'win32';
// apparently you can check if the path in question is an absolute path by platform
pathModule.win32.isAbsolute('C:\\') // --> true
pathModule.posix.isAbsolute('C:\\') // --> false
streamich commented 5 years ago

So the Windows paths work correctly when process.platform === 'win32', in Webpack on Windows process.platform === 'win32' should be true. So it should already work on Windows.

Or is there a requirement in Webpack to treat Windows-like paths on UNIX-like systems as Windows paths?

streamich commented 5 years ago

To write tests for Windows, my first try would be:

  1. Create a separate test suite for Windows.
  2. At the very beginning, before importing anything, set process.platform = 'win32'.
  3. memfs should pick that up and treat all paths as Windows paths.
heygrady commented 5 years ago

I think webpack works platform specific like you are assuming.

I was initially trying to get things to work the way that memory-fs does it where they don’t rely on any system variables and simply detect based on the path itself. They allow you to mix both path types into the same volume. That would work on memfs too if you wanted to change things up but it’s probably not necessary and certainly isn’t how fs does it.

I’ll separate the tests and see how it goes. Thanks!!

heygrady commented 5 years ago

2. At the very beginning, before importing anything, set process.platform = 'win32'.

TypeError: Cannot assign to read only property 'platform' of object '#<process>'
heygrady commented 5 years ago

Is it possible to put an easter egg to enable win32 mode for testing purposes? Like process.env.NODE_ENV === 'test' && process.env.PATH_MODE === 'win32'


Going to try this: https://stackoverflow.com/questions/30405397/how-do-you-mock-process-platform-in-jasmine-tests


Didn't work. The TypeScript gets compiled funky and uses a weird method of reading from process. Overriding process like that isn't going to work. Additionally, I need to use decache to remove memfs from the require.cache at the top of each test. It still doesn't work. See below.

heygrady commented 5 years ago

Hacked my local copy of volume. Still having issues.

// hacked node_modules/memfs/lib/volume.js
var isWin
try {
  if (process_1.default.env.NODE_ENV === 'test' && process_1.default.env.PATH_MODE === 'posix') {
    isWin = false
  } else if (process_1.default.env.NODE_ENV === 'test' && process_1.default.env.PATH_MODE === 'win32') {
    isWin = true
  } else {
    isWin = process_1.default.platform === 'win32';
  }
} catch (e) {}
console.log({isWin}) // --> true
// test.js
process.env.NODE_ENV = 'test' // eslint-disable-line no-process-env
process.env.PATH_MODE = 'win32' // eslint-disable-line no-process-env

const decache = require('decache');
decache("memfs");
const {Volume} = require("memfs");
const fs = new Volume()
fs.mkdirpSync("C:\\test")
console.log(fs.toJSON())
// { '/Users/username/my-test/C:\\test': null }

When I run my script I see:

$ node test.js
{ isWin: true }
{ '/Users/username/my-test/C:\\test': null }
heygrady commented 5 years ago

Ok, more hacking. The problem is how resolve is being set up using unixify from fs-monkey.

  1. unixify is doing its own check for platform.
  2. The way unixify is being used is tripping up resolve.
type TResolve = (filename: string, base?: string) => string;
let resolve: TResolve = (filename, base = process.cwd()) => resolveCrossPlatform(base, filename);
if (isWin) {
  const _resolve = resolve;
  // 1. unixify is checking for platform which make this function a no-op
  const { unixify } = require('fs-monkey/lib/correctPath');
  // 2. the base is posix and so is resolve so I actually need this:
  // unixify(_resolve(unixify(filename), base))
  resolve = (filename, base) => unixify(_resolve(filename, base));
}

Conclusions:

  1. memfs and fs-monkey would need to be refactored to allow for testing win32 paths in a posix test environment.
  2. memfs probably has really good win32 support but it's difficult/impossible to write cross-platform tests.
  3. I was able to hack it to get it working, which is encouraging and allows me to smooth over some rough edges in my win32 implementation but I can't really write tests
// hack
if (isWin) {
   var _resolve_1 = resolve;
   // inline unixify
   var unixify_1 = (filepath, stripTrailing = true) => {
      if(isWin) {
        // inlined this function too (and the rest of correctPath.js)
        filepath = normalizePath(filepath, stripTrailing);
        return filepath.replace(/^([a-zA-Z]+:|\.\/)/, '');
      }
      return filepath;
   };
   resolve = function (filename, base) {
     // unixify the filename when resolve is posix
     if (resolveCrossPlatform === pathModule.posix.resolve) {
       return _resolve_1(unixify_1(filename), base);
     }
     // unixfy the result if we're really win32
     return unixify_1(_resolve_1(filename, base));
   };
}
pizzafroide commented 5 years ago

Is it possible to put an easter egg to enable win32 mode for testing purposes? Like process.env.NODE_ENV === 'test' && process.env.PATH_MODE === 'win32'

Going to try this: https://stackoverflow.com/questions/30405397/how-do-you-mock-process-platform-in-jasmine-tests

Didn't work. The TypeScript gets compiled funky and uses a weird method of reading from process. Overriding process like that isn't going to work. Additionally, I need to use decache to remove memfs from the require.cache at the top of each test. It still doesn't work. See below.

Actually, memfs uses its own process mock (in case there are none for the current environment, like in the browser, for example). It is not currently possible configuring this mock through the "official" interface. Though it is possible hacking the creation of the mock like this:

import * as memfsProcess from 'memfs/lib/process';

memfsProcess.default = memfsProcess.createProcess({});
memfsProcess.default.platform = 'win32';

import memfs from 'memfs';

// ...

I think that allowing users to configure the process mock (among other things) could be valuable. It's been a while that I am thinking about some other improvements (for ensuring the portability of memfs' volume snapshots in testing environments like jest, for example). Unfortunately, I don't have a lot of time right now for getting all that stuff straight, but I hope I'll be able to submit all those ideas soon...

streamich commented 5 years ago

Haven't had time to read this thread properly, I will do it later, but just wanted to mention one thing. Path handling in Node and memfs is done using path module functions. Which internally uses process.platform. memfs uses whatever path module is available: in Node environment it would be the Node implementation, in Webpack it would be whatever shim Webpack replaces it with. (EDIT: or, actually, it should be the same Node implementation.)

So, how paths are handled depends on the path module and whether it picks up the process.platform variable in time. One thing I can think of is there are two path module implementations path.win32 and path.posix, maybe replace path implementation brute-force with Windows one, something like:

const path = require('path');

Object.assign(path, path.win32);
Object.assign(path.posix, path.win32);

Don't know if the above would actually work, maybe better to only replace specific methods used by memfs and fs.

heygrady commented 5 years ago

I’m not well-versed at TypeScript (2019 goal) but I think I’ll take a swing at this and submit a PR. My thinking is that a relatively minor patch to the resolve function is all that’s needed.

We’re really talking about a narrow set of edge cases that are all smoothed over by the resolve function. We simply need to make it aware of a few more situations.

  1. Win32 mode with platform posix
  2. Posix mode with platform win32
  3. Win32 mode with platform win32
  4. Posix mode with platform posix
  5. Bonus: mixed mode, allowing either path type using path.win32.isAbsolute to determine the mode for any given path.

Modes 3 and 4 would be called “strict” mode, and would be the default since that’s how it works today and it matches fs.

Setting a specific “posix/win32” mode would give the resolve function the hints it needs to interpret the paths it receives given the platform.

For instance, someone using memfs in their project could use all posix paths in their code and still have it work cross platform by setting mode to “posix”.

Using “mixed” mode is what memory-fs does where they determine the path mode by inspecting it and making a guess. Mixed mode would have some issue with relative paths but that’s nothing that can be fixed. It would be possible to guess by separator or guess by CWD. This would simply be for compatibility with memory-fs, and they don’t currently allow relative paths anyway.

steinybot commented 2 years ago

I also have a need for this. I have platform specific code that I should be able to test on any platform.

I'm using vitest and I tried using:

            vi.doMock('path', () => {
                return {
                    default: path.win32
                }
            })

but according to https://github.com/vitest-dev/vitest/issues/1336#issuecomment-1131481621 this won't work because memfs does not publish an ESM. If it did then I don't think I wouldn't need any specific behaviour in memfs to do this.

I'm not sure about Jest, but I think it might be possible to do this.

G-Rath commented 2 years ago

I'm thinking it would be good to do a soft reset on this issue, because it's not clear to me the actual problem (it sounds like there could be a few problems, but then we should have an issue for each ideally), and we've had some changes landed since this issue was originally opened which I think might help.

Ultimately my current understanding is that memfs aims firstly to be a user-land implementation of the native fs module, except that it works in-memory rather than with a disk; so whenever we're talking about things like "support win32 paths" we should be doing so as "do what fs does", and currently I believe memfs is matching the behaviour of fs including with paths by being platform-based (i.e. if you're on Windows then paths are handled as win32, otherwise they're handled as posix).

I say this as a framing device: I could very well be wrong, in which case please open a new issue with as much information as possible, including code showing the behaviour of fs vs memfs.

Now this isn't to say that we shouldn't go beyond what fs provides, which I think is what this issue is actually asking for i.e. the ability to have memfs behave as if its running on another platform to what it's actually running on; and that is something I think we can probably support well enough using dependency injection, but I'd like a clearer outline of that request first to be sure its what people actually need and that it'd solve their problems.

golergka commented 1 year ago

I'm thinking it would be good to do a soft reset on this issue, because it's not clear to me the actual problem

I just found this issue when I was debugging windows-path related stuff, and the goal here is simple: to write unit tests for windows-related path functionality without having to spin up an actual windows machine. Memfs is awesome for writing fs unit tests. It would be much more awesome if we could test all windows codepaths in the same breath!

It would be a lifesaver for me, and I would be happy if I just got some guidance by some of the library's maintainers on what would be the best way to implement this.

G-Rath commented 1 year ago

In that case I think we effectively want a sense of "mode" in memfs that controls the path separator and the root that it uses, and probably something along the lines of a clone of our current test suite that uses the win32 mode.

As for the best way to implement it, there's a couple of ways that come to mind but as for which is the best one I don't know right now - so I'd probably start with the internal logic first since that should be more obvious i.e. we're going to want to ensure anytime we do anything with a path separator, it's drawing a single place (you could start with a function that just returns the hardcoded linux separator, that would mean we could easily add logic to have it return a different separator).

caghand commented 4 months ago

There is a general agreement in this thread that Windows-style paths work fine if you are on a Windows machine. While that is mostly true, there is an important exception: watching files.

You can reproduce the problem with the following test code, running on Windows:

const { fs } = require("memfs");
const path = require("path");

const watchedDirectory = path.join(__dirname, "watched");

fs.mkdirSync(watchedDirectory, { recursive: true });

fs.watch(watchedDirectory, { recursive: true }, (eventType, filename) => { console.log("Received: " + eventType + " " + filename); })

fs.writeFileSync(path.join(watchedDirectory, "new_file"), "stuff");

You will see the following output:

Received: rename ../watched/new_file

The correct output should be:

Received: rename new_file

In other words, the file path should be relative to the watched directory. The wrong relative path makes it impossible to generate a proper absolute path for the changed file.

I do not know the best way to solve this. But at first glance, the watchLinkNodeChanged(), onLinkChildAdd() and onLinkChildDelete() functions here are interesting: https://github.com/streamich/memfs/blob/711c4bd24667c88a45551f1bed7a5069d7170dc2/src/volume.ts

You will see a call to relative() in each of these functions. This is the path.posix version of relative(). So, its arguments must be posix paths. With the way the code is written, its second argument is always a posix path. But its first argument is this._filename, which is the original Windows path. This causes very strange behavior in the relative() call. If you would call resolve() on the first argument, it would go through unixify(), and would be ready for the posix relative() call.

Thanks a lot for your consideration!