[FEATURE] Adding input/output feature to Broccoli

[SparshithNR]

This feature is focused to provide better control over the files Read/Write operations. Now input and output property in the plug-in will provide a proxy to the fs library which will allow us to modify the relative path passed to be combined with outputPath generated by broccoli.

Consumer of broccoli plugins need not do fs.writeFileSync(this.outputPath + '/' + 'out.md', 'text'), rather directly call the this.output.writeFileSync('out.md', 'text');

Example:

const Filter = require('broccoli-persistent-filter');

class TestFilter extends Filter {
    constructor(nodes) {
      super(nodes, {
        fsFacade: true
      });
    }

    processString(content) {
        return content.replace(/broccoli/gi, `filter`);
    }
    build() {
      // this.input will have access to all the fs operations except write operations
      let content = this.input.readFileSync('a.txt', 'utf8');
      content = this.processString(content);
      // this.output will have access to all the fs operations.
      this.output.writeFileSync('a.md', content);
    }
};

This is the first step towards informed writing of the outputPath. Once all the write operations go via this.output, we can make a decision whether there was any write operation happened in the build if not there is nothing to update going down the build tree.

Corresponding changes are made in the following repos:

https://github.com/broccolijs/broccoli-node-info/pull/9 https://github.com/broccolijs/broccoli-node-api/pull/7

Needs the node-api changes before this commit. So that we won't get the ts compile-time error.

Tasks

• [ ] bump version for broccoli-node-info
• [ ] bump version for broccoli-node-api

[thoov]

Input API:

this.input.readFileSync('./relative-path');
this.input.existsSync('./relative-path');
this.input.lstatSync('./relative-path');
this.input.readdirSync('./relative-path');
this.input.statSync('./relative-path');

Output API:

this.output.readFileSync('./relative-path');
this.output.existsSync('./relative-path');
this.output.lstatSync('./relative-path');
this.output.readdirSync('./relative-path');
this.output.statSync('./relative-path');
this.output.writeFileSync('./relative-path', 'data');
this.output.appendFileSync('./relative-path', 'data');
this.output.rmdirSync('./relative-path', {});
this.output.mkdirSync('./relative-path');

Expected:

1. Error thrown if absolute path given
2. if unsupported method an error is thrown

[rwjblue]

I don't really see how this can practically be used. No broccoli plugin will be able to rely on the this.input and this.output properties existing, what do we expect them to actually do? Duplicate the implementations (branching on the presence of this.input/this.output)?

[rwjblue]

It is also not clear (from the documentation or implementation) how we expect to actually leverage this code for something useful. I assume providing this.input and this.output in and of itself is not the goal, but instead that we do "something smart" when we know a plugin has been written to. Is that correct? Should the documentation say that?

[SparshithNR]

I don't really see how this can practically be used. No broccoli plugin will be able to rely on the this.input and this.output properties existing, what do we expect them to actually do? Duplicate the implementations (branching on the presence of this.input/this.output)?

This will be initially opt-in feature for all the plugins. So if they opt-in, they can use the this.input and this.output. I am going to add the changes to broccoli-concat and broccoli-funnel soon we merge this.

It is also not clear (from the documentation or implementation) how we expect to actually leverage this code for something useful. I assume providing this.input and this.output in and of itself is not the goal, but instead that we do "something smart" when we know a plugin has been written to. Is that correct? Should the documentation say that?

The goal is once all the broccoli plugins use this.output to write, when there is build in node didn't write anything, there is no need to build further down the build tree. We can just stop there.

[rwjblue]

This will be initially opt-in feature for all the plugins. So if they opt-in, they can use the this.input and this.output. I am going to add the changes to broccoli-concat and broccoli-funnel soon we merge this.

Sure, that part is clear. The point I was making is that those broccoli plugins will have to keep around both sets of logic in order to function, since we do not properly "upgrade" them to always have this.input/this.output.

[rwjblue]

The goal is once all the broccoli plugins use this.output to write, when there is build in node didn't write anything, there is no need to build further down the build tree. We can just stop there.

Yep, I understand that also. However:

1) just because you use this.output to write some changes, doesn't mean you use it for all changes (and we do not currently document / suggest that you should) 2) it is not clear that this implementation allows that functionality 3) adding the assumption mentioned here in my first point just above after releasing the fsFacade option would be a breaking change

[SparshithNR]

1. just because you use this.output to write some changes, doesn't mean you use it for all changes (and we do not currently document / suggest that you should)
2. it is not clear that this implementation allows that functionality

Yes, at this moment we cannot assure that all the plugins will use this method. We may have to release a major version and this.outputPath shouldn't be allowed to access directly in the plugin. As of now, I thought this as a small step towards it. We will incrementally reach that point where all the writing will happen via this.output.

3. adding the assumption mentioned here in my first point just above after releasing the fsFacade option would be a breaking change

At this moment it is not a breaking change, in the future as I mentioned above it will be a breaking change.

[rwjblue]

We may have to release a major version and this.outputPath shouldn't be allowed to access directly in the plugin.

Right, my point is that we should hash out this interop issue and have a plan for it before landing this feature. In this case, a major version bump of Broccoli itself is not good enough, because the intent of the broccoli-node-info/broccoli-plugin APIs is specifically meant to make them work across major versions (and indeed you can see a number of broccoli plugins do test multiple broccoli versions 0.16,0.18,1.0.0, 2.0.0).

[SparshithNR]

Right, my point is that we should hash out this interop issue and have a plan for it before landing this feature. In this case, a major version bump of Broccoli itself is not good enough, because the intent of the broccoli-node-info/broccoli-plugin APIs is specifically meant to make them work across major versions (and indeed you can see a number of broccoli plugins do test multiple broccoli versions 0.16,0.18,1.0.0, 2.0.0).

We expect all the versions of the broccoli to work with all the versions of broccoli-plugins?

[rwjblue]

We expect all the versions of the broccoli to work with all the versions of broccoli-plugins?

Yep, that is exactly what I am saying.

See the test configuration for that here and the latest test run (which includes Broccoli@2.0.0) here.

[SparshithNR]

Yep, that is exactly what I am saying.

See the test configuration for that here and the latest test run (which includes Broccoli@2.0.0) here.

Tests break when I add the new broccoli-source version.

[rwjblue]

Hmm, not sure what you mean. I just added Broccoli 2.1,2.2,2.3,3.0, and 3.1 in https://github.com/broccolijs/broccoli-plugin/pull/42. CI passed on the PR, and is passing on master.

Latest CI run (as of this moment) is green over at https://travis-ci.org/broccolijs/broccoli-plugin/jobs/596235974.

[SparshithNR]

https://github.com/broccolijs/broccoli/pull/440

[rwjblue]

I think this gets closed now that we are going a different direction, right?

[SparshithNR]

Closing as we are taking a different direction.