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

Allow addons to have public trees. #1930

Merged
merged 1 commit into from Sep 8, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 8, 2014

Closes #1625.

@bcardarella
Copy link
Contributor

This concept was previously discussed, at the time it was thought it might be best if the addon copies itspublic/ files into a namespaced public directory. This way if there are commonly named files amongst addons you avoid collisions and other addons can reliably target the namespaced asset.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 8, 2014

Yes, I completely agree with you that it would definitely be a best practice to namespace, but I still find it important to have the ability for an addon to generate files in the root of a project.

A few examples:

  • Generate a sitemap
  • Generating a custom robots.txt (based on logic in the addon or via config)
  • Generating things that are relative to assets/ (things like font-awesome)

@bcardarella
Copy link
Contributor

@rwjblue I think that is OK but the default tree should probably be to namespace then in the event of those usecases (or others) the tree can can be overridden

@bcardarella
Copy link
Contributor

I'm pretty sure if the addon's public/ pushed into the app's global public namespace by default then everyone will just end up doing that instead of any best practice recommendations

@rwjblue
Copy link
Member Author

rwjblue commented Sep 8, 2014

@bcardarella - Sounds reasonable. Will update.

@rwjblue rwjblue force-pushed the addon-public-tree branch 2 times, most recently from 34d9af9 to 3aeba49 Compare September 8, 2014 04:13
@rwjblue
Copy link
Member Author

rwjblue commented Sep 8, 2014

Updated to namespace as discussed.

Also added a default method lookup mechanism that allows for easier customization (without resorting to overriding all of treeFor).

@rwjblue rwjblue force-pushed the addon-public-tree branch 2 times, most recently from f15f198 to fda7c84 Compare September 8, 2014 12:50
* Public is namespaced by default under the addons package name.
* Add simple easy to override method lookup pattern for customizing
  treeFor on a type by type basis.
@rwjblue
Copy link
Member Author

rwjblue commented Sep 8, 2014

And now, finally with passing tests. Those stupid node_modules fixtures being ignored is very annoying.

@rwjblue
Copy link
Member Author

rwjblue commented Sep 8, 2014

@bcardarella - Ready for review, when you have a minute.

@bcardarella
Copy link
Contributor

👍 LGTM

Also appreciate the treeForThing change.

bcardarella added a commit that referenced this pull request Sep 8, 2014
Allow addons to have public trees.
@bcardarella bcardarella merged commit 9e60737 into ember-cli:master Sep 8, 2014
@jas
Copy link

jas commented Sep 17, 2014

@rwjblue In my addon, the treeForPublic method isn't be invoked. I'm using ember-cli 0.0.44. Here are the contents of the addon:

module.exports = {
  name: 'Test Addon',

  init: function (project) {
    this.project = project;
  },

  treeForPublic: function() {
    console.log('treeForPublic invoked');
  }
};

"treeForPublic invoked" is not logged. Any ideas?

@rwjblue
Copy link
Member Author

rwjblue commented Sep 17, 2014

treeForPublic is not in 0.0.44. It will be released in 0.0.45 (hoping today, but this week for sure).

@rwjblue rwjblue deleted the addon-public-tree branch September 17, 2014 21:31
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.

Adding public assets using addons
4 participants