-
Notifications
You must be signed in to change notification settings - Fork 73
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.