fix(@angular/build): allow library outputPath within project root - #34242
Conversation
Remove the upfront check restricting `outputPath` from being within or containing the project root in the library builder. This is needed for Bazel builds where the output directory (`bazel-out/...`) is located under the workspace/project root, and matches the behavior of the application builder which relies on `deleteOutputDir` when cleaning.
There was a problem hiding this comment.
Code Review
This pull request removes the validation check that prevents the outputPath from being equal to, containing, or being located within the projectRoot in the library builder options. A review comment points out that removing this entire validation block could lead to accidental deletion of the project root or workspace (since deleteOutputPath defaults to true), and suggests keeping the safeguards that prevent the output path from being equal to or a parent of the project root.
I am having trouble creating individual review comments. Click here to see my feedback.
packages/angular/build/src/builders/library/options.ts (37-48)
Removing the entire validation block allows the outputPath to be equal to the projectRoot or to contain the projectRoot (i.e., being a parent directory). Since deleteOutputPath defaults to true, running the builder in these configurations could result in the accidental deletion of the entire project root or workspace, leading to severe data loss.
To safely allow the outputPath to be located within the project root (as intended), we should only remove the isSubDirectory(resolvedOutputPath, projectRoot) check, while keeping the safeguards that prevent the output path from being equal to or a parent of the project root.
const outputPath = options.outputPath ?? path.join(workspaceRoot, 'dist', projectName);
const resolvedOutputPath = path.resolve(workspaceRoot, outputPath);
if (
resolvedOutputPath === projectRoot ||
isSubDirectory(projectRoot, resolvedOutputPath)
) {
throw new Error(
`The 'outputPath' (${resolvedOutputPath}) cannot be the project root or contain the project root.`,
);
}
clydin
left a comment
There was a problem hiding this comment.
If the output is in the project root, I think .d.ts files and assets from the built output could potentially end up being picked up by the build. Default generated project shouldn't exhibit this but customizations could unintentionally cause it.
| const outputPath = options.outputPath ?? path.join(workspaceRoot, 'dist', projectName); | ||
| const resolvedOutputPath = path.resolve(workspaceRoot, outputPath); | ||
| if ( | ||
| resolvedOutputPath === projectRoot || | ||
| isSubDirectory(resolvedOutputPath, projectRoot) || | ||
| isSubDirectory(projectRoot, resolvedOutputPath) | ||
| ) { | ||
| throw new Error( | ||
| `The 'outputPath' (${resolvedOutputPath}) cannot be the project root, ` + | ||
| `contain the project root, or be located within the project root.`, | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Probably should keep this safety check but remove isSubDirectory(projectRoot, resolvedOutputPath) and update the error message to cannot be the project root or contain the project root.
There was a problem hiding this comment.
That would break the usage in case it’s a single project repo, and the dist is in the project root.
I can add the check that is not the project root in a follow up, so that this check is also added in the application builder.
There was a problem hiding this comment.
dist in the project root would be fine. Its the case where the project root is inside the output path itself that is problematic since that would delete the entire project root on a build.
|
This PR was merged into the repository. The changes were merged into the following branches:
|
Remove the upfront check restricting
outputPathfrom being within or containing the project root in the library builder. This is needed for Bazel builds where the output directory is located under the workspace/project root, and matches the behavior of the application builder which relies ondeleteOutputDirwhen cleaning.