-
Notifications
You must be signed in to change notification settings - Fork 12k
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(@angular-devkit/core): Rename to a non-existing dir #16839
Conversation
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.
@sacgrover thanks for this, see comment above. Also can you kindly add a unit test in https://github.com/angular/angular-cli/blob/dbaa1be12b4897b130a30a34be0a502125e312ae/packages/angular_devkit/core/node/host_spec.ts and also add Fixes #16484
as a commit footer?
@@ -316,6 +317,9 @@ export class NodeJsSyncHost implements virtualFs.Host<fs.Stats> { | |||
// TODO: remove this try+catch when issue https://github.com/ReactiveX/rxjs/issues/3740 is | |||
// fixed. | |||
try { | |||
if (!fs.existsSync(getSystemPath(path.parse(to).dir as Path))) { |
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.
The castings here are not correct The to
and from
here are devkit paths are they are not interchangeable with FS paths, so you need to get the system path first.
So the logic here should look more like;
try {
const toSystemPath = getSystemPath(to);
if (!fs.existsSync(path.dirname(toSystemPath)) {
fs.mkdirSync(path.dirname(toSystemPath), { recursive: true });
}
fs.renameSync(getSystemPath(from), toSystemPath);
obs.next();
obs.complete();
} catch (err) {
obs.error(err);
}
@alan-agius4 Asked changes done. |
Can you please squash the commits? Otherwise looks good. Thanks. |
Added unit test and requested changes. Fixes #16484
Squashing done. @alan-agius4 |
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.
Thanks LGTM
@alan-agius4 when it will be merged? I am asking because I can see lot of new issues and I want to work on them but by mistake I committed in master branch now If I take a branch out of it then it will not be in synced with original and will have the pending merge changes in it. |
Merged to 9.0.x patch branch as well. We have a caretaking process which merges ready PRs daily, so merging should always be pretty timely after approval and tests pass. Full docs. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Schematics: rename to a non-existing dir fails on sync to disk #16484