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.

Sign up for GitHub

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

Jump to bottom

Allow multiple trees as input to Filter #175

Closed
wants to merge 3 commits into from
Closed

Allow multiple trees as input to Filter #175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbe5f19
Use inout/output feature from broccoli-plugin
SparshithNR Dec 10, 2019
90f7ede
Remove unwated constructor
SparshithNR Dec 10, 2019
53b5ffa
use input.lstatSync
SparshithNR Dec 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 59 additions & 47 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
// @ts-check
'use strict';

const fs = require('fs');
const path = require('path');
const mkdirp = require('mkdirp');
const rimraf = require('rimraf');
const Plugin = require('broccoli-plugin');
const walkSync = require('walk-sync');
const mapSeries = require('promise-map-series');
const symlinkOrCopySync = require('symlink-or-copy').sync;
const debugGenerator = require('heimdalljs-logger');
const md5Hex = require('./lib/md5-hex');
const Processor = require('./lib/processor');
Expand All @@ -18,6 +13,7 @@ const hashForDep = require('hash-for-dep');
const FSTree = require('fs-tree-diff');
const heimdall = require('heimdalljs');
const queue = require('async-promise-queue');
const FSMerger = require('fs-merger');

class ApplyPatchesSchema {
constructor() {
Expand Down Expand Up @@ -45,6 +41,7 @@ class DerivePatchesSchema {
this.entries = 0;
}
}
const FSMERGER = new WeakMap();

const worker = queue.async.asyncify(doWork => doWork());

Expand Down Expand Up @@ -104,8 +101,19 @@ module.exports = class Filter extends Plugin {
return !!result;
}

constructor(inputTree, options) {
super([inputTree], {
get fsMerger() {
Copy link
Collaborator

@stefanpenner stefanpenner Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as implemented fsMerger is still part of the public API for subclasses, this seems sorta strange given the above weakmap usage.

return FSMERGER.get(this).fsMerger;
}

constructor(inputTrees, options) {
// Parsed array of trees to have either broccoliNode or string which is expected by broccoli-plugin
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This processing of inputTrees is surprising, why is this needed?

let inputTreesParsed = [];
inputTrees = Array.isArray(inputTrees) ? inputTrees : [inputTrees];
inputTrees.forEach((tree) => {
inputTreesParsed.push(tree.root ? tree.root : tree);
});

super(inputTreesParsed, {
name: (options && options.name),
annotation: (options && options.annotation),
persistentOutput: true
Expand All @@ -121,6 +129,10 @@ module.exports = class Filter extends Plugin {
loggerName += ' > [' + annotation + ']';
}

FSMERGER.set(this, {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I am worried, that this plugin does not use this.input, did we make a mistake with it? By not using this.input we are making it hard for future optimizations, as ensuring all plugins go through this.input this.output opens many doors when it comes to future optimizations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to make changes to the fs-merger library to somehow match inputPaths and input to broccoli-persistent-filter where we expect input can be broccoli-node string or an object with prefix/getDestinationPath function.

fsMerger: new FSMerger(inputTrees)
});

/** @type {{debug(...s: any[]): void; info(...s: any[]): void}} */
// @ts-ignore
this._logger = debugGenerator(loggerName);
Expand Down Expand Up @@ -158,26 +170,20 @@ module.exports = class Filter extends Plugin {
}

async build() {
// @ts-ignore
let srcDir = this.inputPaths[0];
// @ts-ignore
let destDir = this.outputPath;

if (this.dependencyInvalidation && !this.dependencies) {
this.dependencies = this.processor.initialDependencies(srcDir);
this.dependencies = this.processor.initialDependencies(this.inputPaths);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing this.inputPaths here breaks the encapsulation we hope to to achieve with this.input this.output, is this needed or can the Dependency code be made aware of this.input

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^

}

if (this._needsReset) {
this.currentTree = new FSTree();
// @ts-ignore
let instrumentation = heimdall.start('reset');
if (this.dependencies) {
this.dependencies = this.processor.initialDependencies(srcDir);
this.dependencies = this.processor.initialDependencies(this.inputPaths);
}
// @ts-ignore
rimraf.sync(this.outputPath);
// @ts-ignore
mkdirp.sync(this.outputPath);
this.output.rmdirSync('./', { recursive: true});
this.output.mkdirSync('./', { recursive: true });
instrumentation.stop();
}

Expand All @@ -186,8 +192,8 @@ module.exports = class Filter extends Plugin {
let instrumentation = heimdall.start('derivePatches', DerivePatchesSchema);

let walkStart = process.hrtime();
let entries = walkSync.entries(srcDir);
let nextTree = FSTree.fromEntries(entries);
let entries = this.fsMerger.entries('');
let nextTree = FSTree.fromEntries(entries, { sortAndExpand: true });
let walkDuration = timeSince(walkStart);

let invalidationsStart = process.hrtime();
Expand Down Expand Up @@ -239,27 +245,35 @@ module.exports = class Filter extends Plugin {
let operation = patch[0];
let relativePath = patch[1];
let entry = patch[2];
let outputPath = destDir + '/' + (this.getDestFilePath(relativePath, entry) || relativePath);
let outputFilePath = outputPath;
let fileMeta = this.fsMerger.readFileMeta(relativePath, { basePath: entry.basePath });
let outputPath = relativePath;
if (fileMeta && fileMeta.getDestinationPath) {
outputPath = fileMeta.getDestinationPath(relativePath);
} else {
if (fileMeta && fileMeta.prefix) {
outputPath = fileMeta.prefix + '/' + outputPath;
}
}
outputPath = this.getDestFilePath(outputPath, entry) || outputPath;
let forceInvalidation = invalidated.includes(relativePath);

this._logger.debug('[operation:%s] %s', operation, relativePath);

switch (operation) {
case 'mkdir': {
instrumentation.mkdir++;
return fs.mkdirSync(outputPath);
return this.output.mkdirSync(outputPath, { recursive: true });
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be recursive, the efficient patch based operation are crafted in such a way where no extra IO is required. Making this recursive can introduce unwanted IO, and also hide bugs where the list of operations we are applying was incomplete.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SparshithNR ^^

} case 'rmdir': {
instrumentation.rmdir++;
return fs.rmdirSync(outputPath);
return this.output.rmdirSync(outputPath);
} case 'unlink': {
instrumentation.unlink++;
return fs.unlinkSync(outputPath);
return this.output.unlinkSync(outputPath);
} case 'change': {
// wrap this in a function so it doesn't actually run yet, and can be throttled
let changeOperation = () => {
instrumentation.change++;
return this._handleFile(relativePath, srcDir, destDir, entry, outputFilePath, forceInvalidation, true, instrumentation);
return this._handleFile(relativePath, entry, outputPath, forceInvalidation, true, instrumentation);
};
if (this.async) {
pendingWork.push(changeOperation);
Expand All @@ -270,7 +284,7 @@ module.exports = class Filter extends Plugin {
// wrap this in a function so it doesn't actually run yet, and can be throttled
let createOperation = () => {
instrumentation.create++;
return this._handleFile(relativePath, srcDir, destDir, entry, outputFilePath, forceInvalidation, false, instrumentation);
return this._handleFile(relativePath, entry, outputPath, forceInvalidation, false, instrumentation);
};
if (this.async) {
pendingWork.push(createOperation);
Expand All @@ -292,27 +306,29 @@ module.exports = class Filter extends Plugin {
});
}

async _handleFile(relativePath, srcDir, destDir, entry, outputPath, forceInvalidation, isChange, stats) {
async _handleFile(relativePath, entry, outputPath, forceInvalidation, isChange, stats) {
stats.handleFile++;

let handleFileStart = process.hrtime();
let destDir = this.outputPath + '/' + outputPath;
try {
let result;
let srcPath = srcDir + '/' + relativePath;
let srcDir = this.inputPaths[0]; // keeping this line to maintain the signature of the fn.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using this.inputPaths[0] here is breaking encapsulation, post this.input and this.output migration it should be possible to use only the new APIs, what features are missing or different approaches are required to make this statement true.


if (this.canProcessFile(relativePath, entry)) {
stats.processed++;
if (this._outputLinks[outputPath] === true) {
delete this._outputLinks[outputPath];
fs.unlinkSync(outputPath);
this.output.unlinkSync(outputPath);
}
result = await this.processAndCacheFile(srcDir, destDir, entry, forceInvalidation, isChange, stats);
} else {
stats.linked++;
if (isChange) {
fs.unlinkSync(outputPath);
this.output.unlinkSync(outputPath);
}
result = symlinkOrCopySync(srcPath, outputPath);
// @ts-ignore
result = this.output.symlinkOrCopySync(entry.fullPath, outputPath);
this._outputLinks[outputPath] = true;
}
return result;
Expand Down Expand Up @@ -361,16 +377,7 @@ module.exports = class Filter extends Plugin {
}

isDirectory(relativePath, entry) {
// @ts-ignore
if (this.inputPaths === undefined) {
return false;
}

// @ts-ignore
let srcDir = this.inputPaths[0];
let path = srcDir + '/' + relativePath;

return (entry || fs.lstatSync(path)).isDirectory();
return (entry || this.fsMerger.fs.lstatSync(relativePath)).isDirectory();
}

getDestFilePath(relativePath, entry) {
Expand Down Expand Up @@ -418,10 +425,12 @@ module.exports = class Filter extends Plugin {
if (inputEncoding === undefined) inputEncoding = 'utf8';
if (outputEncoding === undefined) outputEncoding = 'utf8';

let contents = fs.readFileSync(srcDir + '/' + relativePath, {
let contents = this.input.readFileSync(relativePath, {
encoding: inputEncoding
});

let fileMeta = this.fsMerger.readFileMeta(relativePath, { basePath: entry.basePath });

instrumentation.processString++;
let processStringStart = process.hrtime();
let outputString = await invoke(this.processor, this.processor.processString, [this, contents, relativePath, forceInvalidation, instrumentation]);
Expand All @@ -434,10 +443,13 @@ module.exports = class Filter extends Plugin {
relativePath + '") is null');
}

outputPath = destDir + '/' + outputPath;
if (fileMeta && fileMeta.getDestinationPath) {
outputPath = fileMeta.getDestinationPath(outputPath);
}
outputPath = path.join(fileMeta.prefix || '', outputPath);

if (isChange) {
let isSame = fs.readFileSync(outputPath, 'UTF-8') === outputString;
let isSame = this.output.readFileSync(outputPath, 'UTF-8') === outputString;
if (isSame) {
this._logger.debug('[change:%s] but was the same, skipping', relativePath, isSame);
return;
Expand All @@ -447,14 +459,14 @@ module.exports = class Filter extends Plugin {
}

try {
fs.writeFileSync(outputPath, outputString, {
this.output.writeFileSync(outputPath, outputString, {
encoding: outputEncoding
});

} catch (e) {
if (e !== null && typeof e === 'object' && e.code === 'ENOENT') {
mkdirp.sync(path.dirname(outputPath));
fs.writeFileSync(outputPath, outputString, {
this.output.mkdirSync(path.dirname(outputPath), { recursive: true });
this.output.writeFileSync(outputPath, outputString, {
encoding: outputEncoding
});
} else {
Expand Down
Loading