Path resolution fails when changing PWD in contexts, `normalizePath` calls are duplicated.

[james-pre]

normalizePath is used to normalize file URLs, buffers, and irregular strings into valid paths.

Part of this normalization is converting the path into an absolute one via path.resolve. Unfortunatly, normalizePath is not aware of contexts, resulting in path.resolve always using the default context's PWD to resolve relative paths. This was originally pointed out by @jeff-hykin in #261:

One thing though, a bug that would be my first actual PR: I think resolve needs to use this to get the pwd. E.g. this bit of code Right now its always using the default context's pwd. Also, similar bug, normalizePath in utils needs this so it can pass it to resolve. Then, because of that, a lot of normalizePath and resolve calls need to be changed to resolve.call(this, x).

Additionally, calls to normalizePath are duplicated acros multiple code paths. For example, take lstat:

lstat
├ normalizePath
├ realpath
│	├ normalizePath
│	└ _resolve
│		└ resolveMount
│			└ normalizePath
└ resolveMount
	└ normalizePath

That's 4 calls to normalizePath in the best case. Resolving parent directories and symlinks in _resolve further increases the number of calls.

In order to properly fix this problem, the path resolution will be cleaned up to handle everything at once without a mess of function calls and properly handle PWDs.

[james-pre]

I started working on this only to realize the line between VFS and Node.js compat is very blurry. I'm going to go ahead and organize all of the Node.js compatibility under the src/node directory, that way we have a clear separation of concerns.

[jeff-hykin]

Internally I think it would be nice if there was a split between context-less functions (basename, dirname, etc) and context dependant functions (resolve, readFile, etc).

I know, on the outside, we just think of path and fs and utils, but I think it would make bugs easier to catch, and make internal imports more tree-like and less looping-spiderweb-like.

[jeff-hykin]

That's 4 calls to normalizePath in the best case

Yeah... I was kinda thinking about this when you mentioned performance testing.

Speaking of performance. There is a lot of overhead in checking the this over and over inside methods like fs.readFile or path.resolve. Ex: logic like "if not this.pwd, then use defaultContext.pwd". It would be higher performance, and IMO way less prone to bugs (like this issue) if every function was bound to defaultContext before getting exported. Then getting a value like pwd would be as simple as this.pwd.

[james-pre]

I was just checking in on this issue.

if every function was bound to defaultContext before getting exported

This is not possible, since we need to support compatibility with Node.js including archaic things like arbitrary this bindings of functions (e.g. fs.readFileSync.call(false, 'example.txt')), however

There is a lot of overhead in checking the this over and over inside methods like fs.readFile or path.resolve. Ex: logic like "if not this.pwd, then use defaultContext.pwd"

This is true. I think a middle ground would be something like const $ = contextOf(this); to make sure any arbirtrary this binding can be passed. I'm actually working on that right now.

I'm fix the main bug of this issue soon.

[jeff-hykin]

Sounds good! contextOf(this) sounds pretty similar to the getContext(this) from my other PR so I'll be happy 😁 I'll probably completely redo that big PR once this change is done

[james-pre]

Yeah after I implemented it I was like "wait a minute that seems familiar". A lot of it comes down to scale— it's a lot easier to make multiple small changes than one big one. I hope I didn't cause to much inconvenience.

[jeff-hykin]

Oh its fine, I've been on the receiving end and had to do the same thing as a maintainer/owner. I think its better if you hand do it rather than just reading-and-approving someone else's code.