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

Add addon.postprocessTree. #1214

Merged
merged 1 commit into from
Jul 1, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 30, 2014

This hook allows any addon to hook into the final stage of the toTree
process. Making things like broccoli-uglify and broccoli-asset-rev able to
be addons and not need to live in the core of EmberCLI.

Also:

  • Allows additional trees to be passed to app.toTree. Creating
    additional trees in the application Brocfile.js and merging them
    with the result of toTree is pretty common. Unfortunately, this
    means that any additional assets you have added will not be available
    for the post processing in addons. For example, any assets you add
    this way will never be fingerprinted.
  • As the addon API develops it is odd that included and treeFor are
    required to be implemented since many plugins will never interact with
    the EmberApp stuff (for commands, preprocessors, middlewares,
    blueprints (soon), etc). This PR removes the errors from being thrown.
  • Adds broccoli-asset-rev as an application dependency for newly
    blueprinted apps.

broccoli-asset-rev was updated in ember-cli/broccoli-asset-rev#8 and published as 0.0.10.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 30, 2014

@stefanpenner - Before I update broccoli-asset-rev to use this API, I wanted to confirm that you are 👍.

@quaertym
Copy link
Contributor

👍

@rwjblue
Copy link
Member Author

rwjblue commented Jun 30, 2014

@jakecraige - This should allow you do do what we talked about in IRC (post build integration with Cordova). Can you review an confirm?


return addon.treeFor(type);
}, this);
EmberApp.prototype.addonPostprocessTree = function(type, tree) {
Copy link
Member

Choose a reason for hiding this comment

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

You're passing in type here but not using it. Is there a reason for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I intend to have 'all', 'js', and 'css', then move the minification all into addons.

Copy link
Member Author

Choose a reason for hiding this comment

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

But for now, I just added 'all' (to get in the door with something useful). Having the param there for now, means I don't have to break the API when I do flesh it out more.

Copy link
Member

Choose a reason for hiding this comment

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

With that being said, should it be passed into the addon now since that's the plan? So the addons won't have to be updated when it changes

Copy link
Member Author

Choose a reason for hiding this comment

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

Oiy. You are correct. I forgot to pass it. Good catch, thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed now, thanks again for catching it.

@jakecraige
Copy link
Member

@rjackson this is actually different from what I need for my use. I need to hook into the post rebuild process is well. I can talk it over more in IRC if you want. I could put together a PR if it seems useful for other people. I want to hook into the builder's .build method which sane uses and chain on some extra promises. If it needs to be non-blocking they can just use an immediate Promise.resolve

edit:
created a pr for what I need #1215

@rwjblue
Copy link
Member Author

rwjblue commented Jul 1, 2014

@jakecraige - You definitely can do what you are looking for with this API:

function postprocessTree(type, tree) {
  return {
    read: function(readTree) {
      return readTree(tree).then(function(srcDir) {
        runMyAwesomePostBuildCommand();
        return srcDir;
      }
    }
  }
}

That said, your PR makes it simpler for your use case for sure.

@jakecraige
Copy link
Member

@rjackson From the looks of it, doesn't this only run after the initial build? Not after rebuilds?

@rwjblue
Copy link
Member Author

rwjblue commented Jul 1, 2014

@jakecraige - Nope, the read function on every tree is called for every rebuild. This a fundamental part of the Broccoli plugin API.

@jakecraige
Copy link
Member

@rjackson I guess I was logging in the wrong place when I was looking at it, lol. Good to know, thanks

@rwjblue
Copy link
Member Author

rwjblue commented Jul 1, 2014

I do think we need both, one for broccoli based post-processing (like minification, fingerprinting, etc), and the other as a more general purpose post-build thing (which #1215 does well).

@rwjblue rwjblue changed the title [WIP] Add addon.postprocessTree. Add addon.postprocessTree. Jul 1, 2014
@rwjblue
Copy link
Member Author

rwjblue commented Jul 1, 2014

OK, so the updated broccoli-asset-rev has been published, and we now have complete parity (our smoke tests confirm that we still get fingerprinted assets).

@stefanpenner - This is ready to go if you are +1.

@stefanpenner
Copy link
Contributor

needs rebase now, but LGTM

This hook allows any addon to hook into the final stage of the `toTree`
process. Making things like `broccoli-uglify` and `broccoli-asset-rev` able
to be addons and not need to live in the core of EmberCLI.

Also:

* Allows additional trees to be passed to `app.toTree`. Creating
  additional trees in the application `Brocfile.js` and merging them
  with the result of `toTree` is pretty common. Unfortunately, this
  means that any additional assets you have added will not be available
  for the post processing in addons. For example, any assets you add
  this way will never be fingerprinted.
* As the addon API develops it is odd that `included` and `treeFor` are
  required to be implemented since many plugins will never interact with
  the `EmberApp` stuff (for commands, preprocessors, middlewares,
  blueprints (soon), etc). This PR removes the errors from being thrown.
* Adds `broccoli-asset-rev` as an application dependency for newly
  blueprinted apps.
rwjblue added a commit that referenced this pull request Jul 1, 2014
@rwjblue rwjblue merged commit 38e351e into ember-cli:master Jul 1, 2014
@rwjblue rwjblue deleted the allow-addons-to-postprocess branch July 1, 2014 13:24
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.

None yet

5 participants