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

Ensure current working directory files do not affect broccoli-funnel operation #136

Merged
merged 4 commits into from
May 3, 2021

Conversation

ef4
Copy link
Contributor

@ef4 ef4 commented Mar 30, 2021

A directory in process.cwd() with the same name as the specified srcDir will fake out this line:

let inputPathExists = this.input.existsSync(this.srcDir || './');

Given the funnel's that Embroider is doing:

new Funnel(someInput, {
  allowEmpty: true,
  srcDir: 'vendor',
  destDir: 'vendor',
});

If you run that code from within a directory that has a vendor folder the check in Funnel linked above (inputPathExists) is true when it should be false.

The real issue is over in https://github.com/SparshithNR/fs-merger/blob/57b5bc5a6131b848fe8bf982261d676cfb85654e/src/index.ts#L66-L102. Working on failing tests over there now.


References:

Using srcDir, allowEmpty, and destDir together with a missing srcDir used to output an empty destDir, but as of 3.0 it emits nothing. I think this is probably unintentional.

This PR includes a failing test.
ef4 added a commit to ef4/broccoli-funnel that referenced this pull request Mar 30, 2021
 - I need the newer walk sync because it handles symlink cycles
 - there's a newer broccoli-funnel that already updated, but that one is full of bugs (broccolijs#135 broccolijs#136) and I don't have time right now to unfuck it.
@rwjblue rwjblue changed the title allowEmpty destDir regression Add test for allowEmpty along with missing srcDir when using a destDir Apr 15, 2021
@rwjblue
Copy link
Member

rwjblue commented Apr 15, 2021

OK, I've figured out the root cause. Basically a directory in process.cwd() with the same name as the specified srcDir will fake out this line:

let inputPathExists = this.input.existsSync(this.srcDir || './');

Given the funnel's that Embroider is doing:

new Funnel(someInput, {
  allowEmpty: true,
  srcDir: 'vendor',
  destDir: 'vendor',
});

If you run that code from within a directory that has a vendor folder the check in Funnel linked above (inputPathExists) is true when it should be false.

The real issue is over in https://github.com/SparshithNR/fs-merger/blob/57b5bc5a6131b848fe8bf982261d676cfb85654e/src/index.ts#L66-L102. Working on failing tests over there now.

@rwjblue rwjblue added the bug label May 3, 2021
@rwjblue rwjblue changed the title Add test for allowEmpty along with missing srcDir when using a destDir Ensure current working directory files do not affect broccoli-funnel operation May 3, 2021
@rwjblue
Copy link
Member

rwjblue commented May 3, 2021

I've bumped broccoli-plugin to ensure that we have at least fs-merger@3.2.1, and the tests pass now.

@rwjblue rwjblue merged commit 010c5f4 into broccolijs:master May 3, 2021
@ef4 ef4 deleted the bug-report branch October 21, 2021 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants