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

Make inputTrees immutable #1

Closed
caitp opened this issue Jun 5, 2015 · 4 comments
Closed

Make inputTrees immutable #1

caitp opened this issue Jun 5, 2015 · 4 comments

Comments

@caitp
Copy link

caitp commented Jun 5, 2015

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.

@joliss
Copy link
Member

joliss commented Jun 5, 2015

Yes, trees are definitely supposed to be immutable, and we're not enforcing that at the moment.

@stefanpenner mentioned some use cases where Ember CLI addons want to modify the set of inputTrees at initialization time (when Brocfile.js runs basically, so before the Broccoli builder kicks in). I'm still unsure - we could allow early mutation and then freeze it, or maybe we should find a different approach and really have trees be completely immutable. /cc @rwjblue

@caitp
Copy link
Author

caitp commented Jun 5, 2015

Allowing early mutations would be acceptable once outputPaths are fully immutable (the other issue), but I think it still produces code which more difficult to reason about than it should be

@stefanpenner
Copy link
Contributor

Cc @chadhietala

@stefanpenner
Copy link
Contributor

This has gone stale, I believe our current architecture addresses many of these concerns.

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

No branches or pull requests

3 participants