-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Included broccoli-asset-rev for fingerprinting in EmberApp #814
Conversation
@@ -351,9 +363,11 @@ EmberApp.prototype.toArray = function() { | |||
}; | |||
|
|||
EmberApp.prototype.toTree = memoize(function() { | |||
return mergeTrees(this.toArray(), { | |||
var mergeTree = mergeTrees(this.toArray(), { |
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.
what about doing this just when environment is production? like we do with minify?
This now only fingerprints when |
@rickharrison looking great, can you please rebase and also update the Changelog? |
How about making |
Fully rebased and squashed. I also added options for the extensions. |
@rickharrison thanks! could you please also update gh-pages branch so people know how to use this? |
Rad I would love some tests to cover this, eg. ensure the output of production build is valid. I understanding though if you are unable to, we don't really have a great setup for testing that ATM |
👍 this is amazing! thanks! |
Yea, I would like to as well. I'm not the best at writing tests so any guidance here would be appreciated. |
Docs in #824 |
return this.fingerprint(tree); | ||
} else { | ||
return tree; | ||
} |
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.
I know we have this kind of returns in other parts of the code, but I'd prefer we have a single return point, it think it makes the code easier to follow and probably extend in case we want to do any other kind of preprocessing.
So instead of
if (this.env === 'production') {
return this.fingerprint(tree);
} else {
return tree;
}
I'd rewrite like
if (this.env === 'production') {
tree = this.fingerprint(tree);
}
return tree;
This is mostly personal taste, but to that kind of code feels like GOTO's time ;(.
cc @rjackson @stefanpenner ?
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.
I actually prefer the 2nd way like you mentioned. I just wanted to match the style of other areas in the file.
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.
@rickharrison go with it :)
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.
Updated!
@rickharrison thanks, let a final comment, if others are fine with it I'll merge. Really appreciate your work on this 👏 |
@rickharrison about the tests, you can have a look at the smoke tests, that could be a good place for testing this. |
Just added a first take at a smoke test |
@rickharrison LGTM 👍 @stefanpenner @rjackson thoughts? |
@rickharrison Howdy! Trying to get the prepending to work on image urls inside our css files. For some reason the regex is not picking it up. Css looks like the following: Any help would be appreciated ;) |
@rickharrison got time for a rebase? Would love to include this in the next release. |
@@ -352,6 +359,15 @@ EmberApp.prototype.import = function(asset, modules) { | |||
} | |||
}; | |||
|
|||
EmberApp.prototype.fingerprint = function(tree) { | |||
return assetRev(tree, { |
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.
mind changing the alignment?
return assetRev(tree, {
fingerprintExtensions: this.options.fingerprint.extensions,
fingerprintExclude: this.options.fingerprint.exclude,
replaceExtensions: this.options.fingerprint.replaceExtensions,
prependPath: this.options.fingerprint.prepend
});
@pherefordATK I have been unable to reproduce your issue. Are you currently running ember-cli off my fork with npm link? |
@stefanpenner rebased @twokul Alignment changes made Currently, I am going to try and carve out some time this weekend to add tests to the broccoli-asset-rev repo. |
@rickharrison No sir. I was running v0.0.28 and trying to bring in asset-rev as a broccoli plugin. Looking into using your fork now. |
@rickharrison Gotcha. Thank you sir! Sorry for hijacking the thread. |
@pherefordATK Actually, yea disregard what I was saying. You can just bring in my plugin and integrate it with your Brocfile now. Hit me up on irc (rickharrison @ freenode) and I can help you through it. |
@rickharrison travis says you are missing a semicolon in smoke-test ;) |
waiting for travis to pass and merging after |
Included broccoli-asset-rev for fingerprinting in EmberApp
after grabbing latest, I'm getting Cannot find module 'broccoli-asset-rev'Error: Cannot find module 'broccoli-asset-rev' even though I've installed this via npm. Any clues as to why? |
@drewcovi npm install in your |
ha! just traced it down by finding the reference to the broccoli-asset-rev in the ember-cli's package.json. came back here to say "disregard"... but you're far too quick! thanks stefan! |
@rickharrison awesome, thanks so much for this!! 🍻 |
I have included https://github.com/rickharrison/broccoli-asset-rev to both fingerprint files as well as replace them in the source. This will turn:
<script src="assets/appname.js"></script>
into
<script src="assets/appname-ce5adb2fb916f0fdaa16c282dd074239.js"></script>
I added an option to EmberApp to pass strings for excluding files to be fingerprinted.
EmberApp.options.fingerprint.exclude
should be an array of strings, which if any file contains any string in that array, it will not be fingerprinted. As an example, I do not fingerprint my typography.com stylesheets.Also, there is a
prepend
option, which is an object which pairs the current env to a string to add to all of the asset filenames.If a prepend string is specified, it will be added in front of all of the fingerprinted assets. This allows for easy CDN addition to assets.
From
background: url('/images/foo.png');
to
background: url('https://subdomain.cloudfront.net/images/foo-735d6c098496507e26bb40ecc8c1394d.png');
Let me know what you think and I can improve upon it!