-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
fix: copyTemplate failed if tmpdir set to false #1657
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1657 +/- ##
==========================================
- Coverage 92.03% 91.90% -0.13%
==========================================
Files 16 16
Lines 854 865 +11
Branches 167 171 +4
==========================================
+ Hits 786 795 +9
- Misses 51 52 +1
- Partials 17 18 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your PR!
It would be better if you could explain in detail in the PR why this issue occurred. From the existing PR description and code changes, it is difficult for me to understand the reason behind them. I only understood the reason after reviewing the source code of fs.copy (because the condition
is checked before the filter
).
if (!include) return; | ||
|
||
return fs.copy(srcItem, destItem, { | ||
filter, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter, |
Since the judgment has already been made before, there is no need to let fs.copy execute the judgment again.
filter: userPathFilter(this.opts), | ||
dereference: this.opts.derefSymlinks, | ||
}); | ||
if (this.opts.tmpdir === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ianho, we discussed this in an Ecosystem WG meeting and I think this PR is the best solution for https://github.com/electron/forge/issues/3475 as well since it resolves the problem upstream.
Two blockers here:
- Can we add a comment explaining why this is necessary about line 156?
- We should add a test for this code so it doesn't regress in the future.
Summarize your changes:
if tmpdir set to false,
fs-extra copy
api will throw a error thatCannot copy 'xxx' to a subdirectory of itself, 'xxx/out/xxx'.
so we cant call
fs.copy
in this case, traverse first-level directories to callfs.copy
instead