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

Support win32 paths #327

Open
heygrady opened this issue Feb 10, 2019 · 15 comments
Open

Support win32 paths #327

heygrady opened this issue Feb 10, 2019 · 15 comments

Comments

@heygrady
Copy link

heygrady commented Feb 10, 2019

webpack/webpack-dev-middleware#366 (comment)

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

heygrady commented Feb 11, 2019

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

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

streamich commented Feb 11, 2019

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

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

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

TypeError: Cannot assign to read only property 'platform' of object '#<process>'

@heygrady
Copy link
Author

heygrady commented Feb 12, 2019

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

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

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

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

streamich commented Feb 12, 2019

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

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

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 vitest-dev/vitest#1336 (comment) 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
Copy link
Collaborator

G-Rath commented Nov 3, 2022

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

golergka commented Dec 7, 2022

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

G-Rath commented Dec 8, 2022

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

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

No branches or pull requests

6 participants