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

Refactors to simplify the FS proxy implementation #45

Merged
merged 4 commits into from
Apr 29, 2021
Merged

Refactors to simplify the FS proxy implementation #45

merged 4 commits into from
Apr 29, 2021

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Apr 19, 2021

This fixes two major bugs in the prior implementation:

CWD "bleed through"

Prior to these changes the various FS APIs that are wrapped do not properly scope requests to the input source directories.

For example, given this directory structure:

├── bar
│   └── baz
├── foo
└── qux

If the current working directory is the directory above, the following returns the incorrect result:

let merger = new FSMerge(['bar', 'qux');
merger.fs.existsSync('foo');

// -> currently returns `true`, but should obviously be `false

If ran from a working directory that did not have a foo/ folder, it would return the correct result.

References:

Broken "last one wins" semantics

Prior to these changes when using existsSync, lstatSync, or statSync would only work if the first argument to new FSMerger() contained the file in question. Breaking both the fundamental purpose of this library and the "last one wins" semantics.


TODO:

  • Use typescript to help enforce that all valid operations are accounted for
  • Remove defaulting of fullPath = relativePath in handleFSOperation (this will "Fix" the bleed through issue, but causes knock on failures)
  • Include previously failing tests from Failing test demonstrating CWD "bleed through" #43

rwjblue added 2 commits April 29, 2021 15:54
The various FS APIs that are wrapped do not properly scope requests to
the input source directories.

For example, given this directory structure:

```
├── bar
│   └── baz
├── foo
└── qux
```

If the current working directory is the directory above, the following
returns the incorrect result:

```
let merger = new FSMerge(['bar', 'qux');
merger.fs.existsSync('foo');

// -> currently returns `true`, but should obviously be `false
```

If ran from a working directory that did not have a `foo/` folder, it
would return the correct result.
@rwjblue rwjblue marked this pull request as ready for review April 29, 2021 19:57
rwjblue added 2 commits April 29, 2021 16:55
This fixes the bug where directories in the processes current working
directory show up as "existing" when they are not contained by any of
the input paths for the FSMerger.
Prior to this, the three non-overridden APIs (`lstatSync`, `statSync`,
`existsSync`) would actually only work properly if the _first_ tree (as
passed to `new FSMerger()`) contained the file.

This meant that "last one wins" was never possible for those APIs.
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.

2 participants