Skip to content

Allow multiple trees as input to Filter#175

Closed
SparshithNR wants to merge 3 commits intobroccolijs:masterfrom
SparshithNR:in-out-changes
Closed

Allow multiple trees as input to Filter#175
SparshithNR wants to merge 3 commits intobroccolijs:masterfrom
SparshithNR:in-out-changes

Conversation

@SparshithNR
Copy link

@SparshithNR SparshithNR commented Aug 23, 2019

  • Using fs-merger
    1. Allow filter developers to reduce mergeTree or funneling entire array of trees to one tree and then pass it to filter
    2. Allows input array of trees as input.
  • input/output :
    1. Provides good ergonomics and avoids appending the this.outputPath
  • Test:
    1. Added additional tests to make sure that Filter can take more than one tree

Example:

class Awk extends Filter {
  constructor(inputNode, search, replace, options) {
    options = options || {};
    super(inputNode, {
      annotation: options.annotation
    });
    this.search = search;
    this.replace = replace;
    this.extensions = ['txt'];
    this.targetExtension = 'txt';
  }
  processString(content, relativePath) {
    return content.replace(this.search, this.replace);
  };
}
module.exports = new Awk(['fixture','fixture_2'], 'test', 'real');

Since we are now allowing users to pass two paths/nodes. We avoid a uncessary merge/funnel.
Before to achive the above result we had to below sinppet.

let fullTree = mergeTree(['fixture','fixture_2'];
module.exports = new Awk(fullTree, 'test', 'real');

@SparshithNR SparshithNR reopened this Aug 28, 2019
@SparshithNR SparshithNR changed the title [WIP] Using fs-merger module to facilitate array of trees as input to persistent filter Using fs-merger module to facilitate array of trees as input to persistent filter Aug 28, 2019
Copy link
Contributor

@chriseppstein chriseppstein left a comment

Choose a reason for hiding this comment

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

I didn't review everything, but I have enough questions to pause at this point.

@SparshithNR SparshithNR changed the title Using fs-merger module to facilitate array of trees as input to persistent filter Use inout/output feature from broccoli-plugin Dec 6, 2019
Using fs-merger
1. Allow filter developers to reduce mergeTree or funneling entire array of trees to one tree and then pass it to filter
2. Allows input array of trees as input.

input/output :
1. Provides good ergonomics and avoids appending the this.outputPath

Test:
1. Added additional tests to make sure that Filter can take more than one tree

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

@stefanpenner stefanpenner Dec 12, 2019

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.

@SparshithNR SparshithNR changed the title Use inout/output feature from broccoli-plugin Allow multiple trees as input to Filter Dec 12, 2019
@SparshithNR
Copy link
Author

Input output facade is addressed in #187.
We will address allowing multiple trees as input in this PR.

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.

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.


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.

^^

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
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.

@SparshithNR
Copy link
Author

Closing this as #188 is addressing this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants