Better addon support #52
Conversation
This change allows ember-svg-jar to be used by addons. [Recent work](evoactivity#29) also mentioned making it work in addons, but that only makes it work within an addon's own dummy app, when consumed as a `devDependency`. This change makes ember-svg-jar work as a `dependency`. Multiple addons and/or the top-level app can all use it simultaneously without stomping each other (with the one caveat that the namespace of SVG asset ids is flat, so `{{svg-jar "something"}}` has a consistency meaning no matter where it appears. I only implemented this for the default, inline strategy. It would be possible in the future to do for the symbol strategy.
| this.svgJarOptions = buildOptions( | ||
| app.options.svgJar, | ||
| app.env === 'development', | ||
| app.options.trees.public |
There was a problem hiding this comment.
This takes care of automatically finding both public and tests/dummy/public depending on whether we are a normal app or an addon's dummy app.
Sometimes the thing we find here will already be a Broccoli tree (not just a string).
| @@ -150,31 +160,31 @@ module.exports = { | |||
|
|
|||
| sourceDirsFor(strategy) { | |||
| return this.optionFor(strategy, 'sourceDirs') | |||
| .filter((sourceDir) => fs.existsSync(sourceDir)); | |||
| .filter((sourceDir) => typeof sourceDir !== 'string' || fs.existsSync(sourceDir)); | |||
There was a problem hiding this comment.
This was needed because we may sometimes discover that the app's default public tree is already some kind of broccoli output tree and not a plain string.
| }, | ||
|
|
||
| originalSvgsFor: _.memoize(function(strategy) { |
There was a problem hiding this comment.
This doesn't work correctly for instance methods -- it doesn't take this into account, so all instances of the class will share the same memoized value. In our case, this class will get instantiated once for each addon that depends on it, plus possibly once for the containing application (or dummy application).
|
Awesome! Thank you. |
| function loader(assetId) { | ||
| try { | ||
| /* eslint-disable global-require */ | ||
| return require(`ember-svg-jar/inlined/${assetId}`).default; |
There was a problem hiding this comment.
This way we will have multiple HTTP requests, that can slow down the first app load. What do you think?
There was a problem hiding this comment.
This doesn't trigger an HTTP request, it's just loading a module that was already embedded in vendor.js.
(Implementing the symbol strategy for multiple addons on the other hand might cause multiple HTTP requests, which is one reason I didn't get to that yet.)
|
By the way, I think the test failures are caused by a regression in Travis, and you may be able to work around them like this: |
|
@ivanvotti are you going to make a release with this? I had to fork it to get my project rolling. Much appreciated! |
|
@kriswill Sure, I’m going to do that soon. |
|
The feature is available in the latest ember-svg-jar release |
This change allows ember-svg-jar to be used by addons. Recent work also mentioned making it work in addons, but that only makes it work within an addon's own dummy app, when consumed as a
devDependency.This change makes ember-svg-jar work as a
dependency. Multiple addons and/or the top-level app can all use it simultaneously without stomping each other (with the one caveat that the namespace of SVG asset ids is flat, so{{svg-jar "something"}}has a consistent meaning no matter where it appears, so if multiple addons try to use the same id, it's undefined which wins.I only implemented this for the default, inline strategy. It would be possible in the future to do for the symbol strategy.