Skip to content

Commit

Permalink
Normalize all paths used in dependency tracking. (#170)
Browse files Browse the repository at this point in the history
* Not all files that change in the broccoli tree are due to dependencies, so we must handle missing dependents on a changed file.

* Normalize paths to avoid duplicate paths and handle paths using / in windows.

* Add test for path normalization in dependency tracking.
  • Loading branch information
chriseppstein authored and stefanpenner committed Jun 26, 2019
1 parent 06345a7 commit ba41be8
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 6 deletions.
9 changes: 6 additions & 3 deletions lib/dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ module.exports = class Dependencies {
* paths are resolved against this directory.
* @type {string}
*/
this.rootDir = rootDir;
this.rootDir = path.normalize(rootDir);
/**
* Tracks dependencies on a per file basis.
* The key is a relative path, values are absolute paths.
Expand Down Expand Up @@ -133,14 +133,15 @@ module.exports = class Dependencies {
* containing the file that depends on them.
*/
setDependencies(filePath, dependencies) {
filePath = path.normalize(filePath);
if (this.sealed) {
throw new Error("Cannot set dependencies when sealed");
}
/** @type {Array<string>} */
let absoluteDeps = [];
let fileDir = path.dirname(filePath);
for (let i = 0; i < dependencies.length; i++) {
let depPath = dependencies[i];
let depPath = path.normalize(dependencies[i]);
if (!path.isAbsolute(depPath)) {
depPath = path.resolve(this.rootDir, fileDir, depPath);
}
Expand All @@ -160,6 +161,7 @@ module.exports = class Dependencies {
* @returns {Dependencies}
*/
copyWithout(files) {
files = files.map(f => path.normalize(f));
let newDeps = new Dependencies(this.rootDir);
for (let file of this.dependencyMap.keys()) {
if (!files.includes(file)) {
Expand Down Expand Up @@ -219,6 +221,7 @@ module.exports = class Dependencies {
for (let operation of patch) {
let depPath = path.join(fsRoot, operation[1]);
let dependents = this.dependentsMap.get(depPath);
if (!dependents) { continue; }
for (let dep of dependents) {
invalidated.add(dep);
}
Expand Down Expand Up @@ -287,7 +290,7 @@ module.exports = class Dependencies {
*/
static deserialize(dependencyData, newRootDir) {
let oldRootDir = dependencyData.rootDir;
newRootDir = newRootDir || oldRootDir;
newRootDir = path.normalize(newRootDir || oldRootDir);
let dependencies = new Dependencies(newRootDir);
let files = Object.keys(dependencyData.dependencies);
for (let file of files) {
Expand Down
17 changes: 14 additions & 3 deletions test/dependencies-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,10 @@ function pathFor(root, filePath) {
filePath = root;
root = undefined;
}
filePath = filePath.split("/").join(path.sep);
if (root) {
return path.normalize(path.resolve(root, filePath));
return path.resolve(root, filePath);
} else {
return filePath;
return path.normalize(filePath);
}
}

Expand Down Expand Up @@ -72,6 +71,18 @@ describe('Dependency Invalidation', function() {
]);
});

it('normalizes paths', function() {
let dependencies = new Dependencies(path.join(__dirname, 'fixtures', 'something', '..', 'dependencies')); // always uses path.sep and contains a parent directory.
dependencies.setDependencies('othersubdir/../subdir/subdirFile1.txt', [ // always passes unix paths and contains a parent directory.
path.join(DEP_FIXTURE_DIR, 'thirdSubdir/../subdir/subdirFile2.txt'), // mixes windows and unix paths and has a parent directory.
path.join(EXT_DEP_FIXTURE_DIR, 'dep-1.txt')
]);
assert.deepEqual(dependencies.dependencyMap.get(pathFor('subdir/subdirFile1.txt')), [
pathFor(DEP_FIXTURE_DIR, 'subdir/subdirFile2.txt'),
pathFor(EXT_DEP_FIXTURE_DIR, 'dep-1.txt')
]);
});

it('relative dependencies are relative to the file', function() {
let dependencies = new Dependencies(DEP_FIXTURE_DIR);
dependencies.setDependencies(pathFor('subdir/subdirFile1.txt'), [
Expand Down

0 comments on commit ba41be8

Please sign in to comment.