broccolijs / broccoli-plugin Goto Github PK

See [https://github.com/ember-cli/broccoli-caching-writer/issues/70] for background.

I think the bug here might be a bug/design-flaw in broccoli. It appears (from my very much outsider view as a plugin writer) that a new tmp/cache and tmp/output directory is created on each run of broccoli. This isn't inline with the stated aim of the persistentOutput option - what's the point of keeping outputs around for a subsequent run and then creating a new folder and not reusing the previous output?

Would it be better if the tmp\xxx directory name was predictable, e.g. using classname+instance number or annotation and was only cleared down if the BrocFile was changed? This could be done by storing a hash of the brocfile in the tmp folder and checking it on startup.

Plugins could now reuse their previous output as per the broccoli design goal.

Thoughts? @joliss @rwjblue @stefanpenner

Hi Guys,

since the last update we get
BroccoliMergeTrees (TreeMerger (lint app)): Expected Broccoli node, got [object Object] for inputNodes[1]

not sure if it's related to the last update or not, did anyone encouter the same? any reason this might happen?

https://snyk.io/test/github/broccolijs/broccoli-plugin

Regular Expression Denial of Service (ReDoS)
Vulnerable module: underscore.string
Introduced through: [email protected]
Detailed paths
Introduced through: broccoli-plugin@broccolijs/broccoli-plugin › [email protected] › [email protected]

Same issue available in broccoli-plugin version 2.* & 3.*

Trying to run the sample plugin against broccoli-plugin-1.2.1 fails with the following error:

/home/gareth2/dev/react-redux-tutorial/node_modules/broccoli-plugin/index.js:5
  if (!(this instanceof Plugin)) throw new TypeError('Missing `new` operator')
                                 ^

TypeError: Missing `new` operator
    at Plugin (/home/gareth2/dev/react-redux-tutorial/node_modules/broccoli-plugin/index.js:5:40)
    at MyPlugin (/home/gareth2/dev/react-redux-tutorial/broccoli-plugins/babel.js:10:10)
    at Object.<anonymous> (/home/gareth2/dev/react-redux-tutorial/Brocfile.js:10:10)
    at Module._compile (module.js:434:26)
    at Object.Module._extensions..js (module.js:452:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.loadBrocfile (/home/gareth2/dev/react-redux-tutorial/node_modules/broccoli/lib/builder.js:245:14)

My guess is that the if (!(this instanceof Plugin)) throw new TypeError('Missingnewoperator') broke the example when it was added?

(I've attached the sample as it was when I encountered this error below).

var Plugin = require('broccoli-plugin');
var path = require('path');

// Create a subclass MyPlugin derived from Plugin
MyPlugin.prototype = Object.create(Plugin.prototype);
MyPlugin.prototype.constructor = MyPlugin;
function MyPlugin(inputNodes, options) {
  options = options || {};
  Plugin.call(this, inputNodes, {
    annotation: options.annotation
  });
  this.options = options;
}

MyPlugin.prototype.build = function() {
  // Read files from this.inputPaths, and write files to this.outputPath.
  // Silly example:

  // Read 'foo.txt' from the third input node
  var inputBuffer = fs.readFileSync(path.join(this.inputPaths[2], 'foo.txt'));
  var outputBuffer = someCompiler(inputBuffer);
  // Write to 'bar.txt' in this node's output
  fs.writeFileSync(path.join(this.outputPath, 'bar.txt'), outputBuffer);
};

Let's say I'm writing a plugin for a preprocessor language of my choice which can import files by relative file path. My directory layout looks like:

root/
  lib/
    other.file
  src/
    main.file

and main.file imports other.file and I need to compile the tree rooted at root/src. What is the recommended behaviour in this case?

My plugin doesn't know what the original path of main.file is -- it only knows the path to its input nodes (which are in tmp). In this case, it can't find other.file (which is being imported as e.g. ../lib/other.file). Is the intended behaviour for the compilation to fail? Is there a way we can get the original file paths, or annotate the input nodes with those paths?

If the promise, returned by plugin, is rejected, broccoli fails without proper message, but with the following:

Error: ENOTEMPTY, directory not empty
    at Error (native)
    at Object.fs.rmdirSync (fs.js:711:18)
    at rmkidsSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:247:11)
    at rmdirSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:237:7)
    at fixWinEPERMSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:150:5)
    at rimrafSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:216:26)
    at <PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:245:5
    at Array.forEach (native)
    at rmkidsSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:244:26)
    at rmdirSync (<PLUGIN_ROOT_HERE>\node_modules\broccoli-plugin\node_modules\quick-temp\node_modules\rimraf\rimraf.js:237:7)

Win7 x86_64, node 0.12.5.

instead of sync cleanup here: https://github.com/broccolijs/broccoli-plugin/blob/master/read_compat.js#L69-L71

We should consider:

1. check if $TMPDIR is writable
2. if isn't, continue as today
3. if it is, move file to be deleted to $TMPDIR
4. then delete

This will prevent the, early exit leaving tmp in a messy state, instead deferring the messy state to OS TMPDIR periodic cleanup.

Thoughts? Something like: https://github.com/stefanpenner/move-to-tmp-and-remove/blob/master/index.js#L1-L21

I believe the issue is due to spawn-cmd which multidep uses internally.

CI against Node 15 passed when using 15.11.0 and fails against 15.14.0.

According to documentation:
"this.cachePath: The path on disk to an auxiliary cache directory. Use this to store files that you want preserved between builds. This directory will only be deleted when Broccoli exits."

Is it possible to preserve cachePath between without watching?

I really like the new Plugin base class, however I think I'm running up against some things that feel bad. It may be that use case is rare but I thought I would share.

For Ember CLI I've been working on re-writing the build pipeline so that it actually resolves a dependency graph. It uses a series of broccoli plugins to get this done. There are several cases where I would like to know more information about what the tree represents other than it's inputPath or having to walkSync and iterate over it's contents. A more concrete example is something like the following.

var linkedTree = new Linker([ 'tree1', 'tree2', 'tree3'], {
   meta: [
     { treeName: 'tree1', root: '/some/path/in/the/project', nodeModulesPath: '/some/node_modules/path' },
     { treeName: 'tree2', root: '/some/path/in/the/project', nodeModulesPath: '/some/node_modules/path' },
     { treeName: 'tree3', root: '/some/path/in/the/project', nodeModulesPath: '/some/node_modules/path' }
  ] 
})

Internally I'm relying on the fact that the trees and the meta are ordered sets so that when I iterate through either the inputPaths or inputNodes I can do a lookup in the meta based on the index. So far it's worked out pretty well, but it seems like there might be room to have some primitive for something like this. I've thought about turning both the options and things set by broccoli into immutable data structures on instantiation so I have guarantees about mutation. I trust that broccoli internally will not mutate the inputPaths array but it's just a precaution.

I will admit this is probably one of the most complex broccoli plugins and may just be "doable" but out of the happy path scope.

Reading API updates regarding this.input, this.output. I'was checked original broccoli plugin documentation on website and seen such example:

https://github.com/broccolijs/broccolijs.github.io/edit/code/src/content/plugins.md

build() {
        const walkOptions = {
            includeBasePath: true,
            directories: false,
            globs: this.fileMatchers,
        };

        const content = this.inputPaths
            .reduce((output, inputPath) => output + this.joinSeparator +
                walkSync(inputPath, walkOptions)
                    .map(file => fs.readFileSync(file, { encoding: 'UTF-8' }))
                    .join(this.joinSeparator),
            '');

        fs.writeFileSync(`${this.outputPath}/${this.outputFile}`, content);
    }

How we will convert it into new api? without fs usage inside walkSync?

// @SparshithNR @stefanpenner

I am currently trying to debug this issue:
fossasia/open-event-frontend#7971

I have a plugin that looks basically like this:

class CreateEmberL10nFastBootAssetMap extends Plugin {
  constructor(
    inputNode
  ) {
    super([inputNode], {});
  }

  build() {
    // We only support passing in one input path (for simplicity)
    this.inputPath = this.inputPaths[0];

    this.parseNode(this.inputPath);

    this.createFastBootAssetMapModule();
  }

  parseNode(inputPath) {
    let stat = this.input.statSync(inputPath);

    if (stat.isFile()) {
      this.parseFile(inputPath);
    } else if (stat.isDirectory()) {
      this.parseDirectory(inputPath);
    }
  }

  parseDirectory(inputPath) {
    let outputPath = this._getOutputPath(inputPath);
    if (!this.output.existsSync(outputPath)) {
      this.output.mkdirSync(outputPath);
    }

    let files = this.input.readdirSync(inputPath);
    files.forEach((file) => this.parseNode(path.join(inputPath, file)));
  }

  parseFile(inputPath) {
    let content = this.input.readFileSync(inputPath);
    let outputPath = this._getOutputPath(inputPath);

    // logic goes here
  }

  _getOutputPath(inputPath) {
    return path.relative(this.inputPath, inputPath);
  }
}

However, it seems that let inputPath = this.inputPaths[0]; is an absolute path to a temp directory here, e.g.:

/tmp/broccoli-3137903S9AmcZtiodI/out-0999-append_ember_auto_import_analyzer/

And this.input.readdirSync or this.input.readFileSync will convert that into e.g.

/tmp/broccoli-3137903S9AmcZtiodI/out-0999-append_ember_auto_import_analyzer//tmp/broccoli-3137903S9AmcZtiodI/out-0999-append_ember_auto_import_analyzer

Which obviously does not exist. To be honest I am not quite sure where that comes from exactly, but somewhere there seems to be a missing handling for absolute paths, I guess?

If your app is not using broccoli@^3.2.0, the trackInputChanges option does nothing, meaning that the build() method does not get a list of changes. There is no warning or error shown.

cc @thoov

Every time I save my files, it throws: "Error: Closing tag div (on line 28) without an open tag". I deleted that div, and it gave me another error saying that its parent element didn't have a closing tag either. I may as well delete my entire HTML and it will still give me that error. I know there's no mistake because Atom says there are no issues with my code. What can I do?

When a plugin accepts multiple inputTrees, the inputTrees may be mutated outside of the stream of the build. This leads to instability which is difficult to manage, and it's a bit challenging to poke through a build and figure out why it's behaving in a weird way that it does.

A comment from angular/angular#2064 (comment) should help to clarify why this is a problem.

An example fix for this (which would prevent code as linked above from being written in the first place):

function BasePluginClass(inputTrees, options) {
  if (Array.isArray(inputTrees)) {
    // No pushing/popping/splicing/etc, throw errors in strict mode when trying to adjust these
    Object.preventExtensions(inputTrees);
    Object.defineProperty(this, "inputTrees", readOnlyNonConfigurable(inputTrees));
  } else {
    Object.defineProperty(this, "inputTree", readOnlyNonConfigurable(inputTrees));
  }
  // ... whatever else
}

Maybe there's a better way to do this to give a hint to make sure we don't write broccoli scripts in stupid ways, or something, but this is the first thing that comes to mind.

This is related to the mutable outputPath issue discussed previously, but is somewhat different in practice.

own files we may not want:

• tmp 110

deps we may want to investigate

• promise-map-series 50
• quick-temp 68

this isn't ideal with slow/non-existent net. Is it possible for it not to rely on it, or maybe have an offline mode?