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.

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

[FEATURE] Input node change tracking #419

Merged
merged 8 commits into from
Aug 14, 2019

Conversation

thoov
Copy link
Member

@thoov thoov commented Jul 30, 2019

Background

This PR adds opt-in input node change tracking. If enabled, Broccoli will pass an object as the first argument into the build method of plugins when invoking them. We are passing an object as it will give us greater flexibility to add additional properties later.

Implementation

The type of this object is:

interface BuildObject {
  changedNodes: boolean[]
}

By default this will not be passed into the build method. To enable this feature, plugins must pass trackInputChanges:

class MyPlugin extends Plugin {
  constructor(inputNodes) {
    super(inputNodes, {
      trackInputChanges: true
    });
  }
}

Use Case

The primary use case for this feature is to enable "selective building". An example of this is if your plugin takes several inputs and performs some action on each inputPath. This can become inefficient with either a large number of inputs or if your action is expensive. Currently plugin authors have to create their own diffing solution which generally is inefficient due to having to touch the filesystem. This feature leverages the revision tracking system and thus can efficiently inform plugins on which node has changed. A concrete example of where it will be used is in embroider to allow addons to be rebuilt: embroider-build/embroider#297.

Example

const fs = require("fs");
const Plugin = require("broccoli-plugin");
const { WatchedDir } = require("broccoli-source");

class MyPlugin extends Plugin {
  constructor(inputNodes) {
    super(inputNodes, {
      trackInputChanges: true
    });
  }

  build({ changedNodes }) {
    this.inputPaths.forEach((path, idx) => {
        if (changedNodes[idx]) {
          const input = fs.readFileSync(`${this.inputPaths[idx]}/foo.txt`);
          fs.writeFileSync(`${this.outputPath}/${idx}-foo.txt`, input);
        }
    });
  }
}

export default () => {
  let app = new WatchedDir("app");
  let foo = new WatchedDir("foo");

  return new MyPlugin([app, foo]);
};

lib/wrappers/transform-node.js Outdated Show resolved Hide resolved
@thoov
Copy link
Member Author

thoov commented Aug 8, 2019

@oligriffiths could you take a look at this

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I don't fully grok the goal / intent of the change here, maybe a test would help?

obj[idx] = revision ? revision.changed : true;
});

resolve(this.callbackObject.build({ changedNodes: obj }));
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this change also require a corresponding change to the TS interfaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it would require a change

@thoov
Copy link
Member Author

thoov commented Aug 9, 2019

@rwjblue The goal is that if you are a plugin that does something on each one of your inputNodes there isn't a great way to know which one of them changed on a rebuild. This exposes the ability to easily know which one of your inputs have changed instead of having to come up with a diffing algo.

@oligriffiths
Copy link
Contributor

Seems the right direction.
I wonder if it would be better or not for the object passed to expose as hasChanged(inputPath) method or not. Throwing it out as a suggestion

obj[idx] = revision ? revision.changed : true;
});

resolve(this.callbackObject.build({ changedNodes: obj }));
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this object should expose an interface like:

class BuildContext
{
  changedNodes: [],
  hasInputNodeChanged(idx) {
    return !!this.changedNodes[idx];
  }
}

This way we can alter the data structure and interface of this object as we see fit.
We could potentially pass other info in here, like the build/rebuild number, if it's the initial or a rebuild, etc.

Just thinking out loud rn

Copy link
Member Author

Choose a reason for hiding this comment

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

Im not sure how much this abstraction gives us. Im not against a method like hasInputNodeChanged but it feels weird to have that as what is passed into the build method.

@thoov thoov changed the title [WIP] Enable plugins the ability to determine which input node has changed Enable the ability to determine which input node has changed Aug 12, 2019
@thoov thoov changed the title Enable the ability to determine which input node has changed Input node change tracking Aug 13, 2019
@thoov thoov changed the title Input node change tracking [FEATURE] Input node change tracking Aug 13, 2019
@thoov thoov requested a review from krisselden August 13, 2019 18:08
@thoov
Copy link
Member Author

thoov commented Aug 13, 2019

@oligriffiths @rwjblue @krisselden Could you guys review this again. I made it opt-in (per Rob's suggestion), added some tests, and have updated the description.

thoov added a commit to broccolijs/broccoli-node-api that referenced this pull request Aug 13, 2019
Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Overall looks really good!

A few small things:

  • We should add a test that confirms nothing is passed when you 1) explicitly opt out, and 2) when you don't mention trackChangedInputs at all
  • Add information RE: the new NodeInfo to the docs. I don't know off the top of my head if that goes in the README.md or some other file though.

@thoov thoov merged commit 5215182 into broccolijs:master Aug 14, 2019
@thoov thoov deleted the input-node-changed branch August 14, 2019 21:29
thoov added a commit to broccolijs/broccoli-node-api that referenced this pull request Aug 14, 2019
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