Skip to content

Commit

Permalink
Add feature for deduplicating javascript and stylesheets (#205)
Browse files Browse the repository at this point in the history 
* Add a uniq parameter to the asset gsp tags

The idea is that if you have a page that is composed of a bunch of modules, you may want each
module to declare it's own asset dependencies. Often times these dependencies will contain a bunch of
overlapping requirements of common libraries and such, which should not be included multiple times.

If the uniq parameter is supplied, keep track of which assets have been included into the page,
or more specifically in the request, and skip writing out duplicate entries. This approach is
completely incompatible with resource bundling, since the bundler can't know where the overlaps are.

* Ensure that assets are tracked uniformly

When using the memo for tracking which assets have been included so far
we need to make sure that we're always using the same name for each asset
so do that post resolve.

* Track already written assets by both name and extension

Not sure why I thought it was a good idea to track the already included assets by just name
but we need to include the extension as well since you might have, e.g. js and css files with
the same basename.

* Don't write out bundled assets in war deployment

Avoid using the asset manifest when either uniq mode, or bundling is disabled.
The asset manifest, if it exists, will point to the pre-bundled assets, which ultimately
defeat the purpose of those config options.

Also add a global config option to do the same, defaulting to off of course.

* Missed a call site of the output closure
  • Loading branch information
@piis3 @davydotcom
piis3 authored and davydotcom committed May 23, 2018
1 parent 54ab669 commit f8bd3d7
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 40 deletions.
3 changes: 3 additions & 0 deletions ...-pipeline-grails/grails-app/assets/javascripts/asset-pipeline/test/test_simple_require.js
View file
@@ -0,0 +1,3 @@
//=require libs/file_a

console.log("This and file_a should be included");
13 changes: 8 additions & 5 deletions asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetMethodTagLib.groovy
View file
Expand Up @@ -16,16 +16,19 @@ class AssetMethodTagLib {
def assetPath = {final def attrs ->
final def src
final UrlBase urlBase
final boolean useManifest

if (attrs instanceof Map) {
src = attrs.src
urlBase = attrs.absolute ? SERVER_BASE_URL : CONTEXT_PATH
src = attrs.src
urlBase = attrs.absolute ? SERVER_BASE_URL : CONTEXT_PATH
useManifest = attrs.useManifest ?: true
}
else {
src = attrs
urlBase = CONTEXT_PATH
src = attrs
urlBase = CONTEXT_PATH
useManifest = true
}

return assetProcessorService.assetBaseUrl(request, urlBase) + assetProcessorService.getAssetPath(Objects.toString(src))
return assetProcessorService.assetBaseUrl(request, urlBase) + assetProcessorService.getAssetPath(Objects.toString(src), useManifest)
}
}
79 changes: 52 additions & 27 deletions asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetsTagLib.groovy
View file
@@ -1,35 +1,36 @@
package asset.pipeline.grails

import grails.util.Environment
import grails.core.GrailsApplication
import asset.pipeline.AssetPipeline
import asset.pipeline.AssetPipelineConfigHolder
import asset.pipeline.AssetHelper
import asset.pipeline.AssetPipeline
import asset.pipeline.AssetPipelineConfigHolder
import grails.core.GrailsApplication
import org.grails.buffer.GrailsPrintWriter


class AssetsTagLib {

static namespace = 'asset'
static returnObjectForTags = ['assetPath']

static final ASSET_REQUEST_MEMO = "asset-pipeline.memo"
private static final LINE_BREAK = System.getProperty('line.separator') ?: '\n'

GrailsApplication grailsApplication
def assetProcessorService


/**
* @attr src REQUIRED
* @attr asset-defer OPTIONAL ensure script blocks are deferred to when the deferrred-scripts is used
* @attr uniq OPTIONAL Output the script tag for the given resource only once per request, note that uniq mode cannot be bundled
*/
def javascript = {final attrs ->
final GrailsPrintWriter outPw = out
attrs.remove('href')
element(attrs, 'js', 'application/javascript', null) {final String src, final String queryString, final outputAttrs, final String endOfLine ->
element(attrs, 'js', 'application/javascript', null) {final String src, final String queryString, final outputAttrs, final String endOfLine, final boolean useManifest ->
if(attrs.containsKey('asset-defer')) {
script(outputAttrs + [type: "text/javascript", src: assetPath(src: src) + queryString],'')
script(outputAttrs + [type: "text/javascript", src: assetPath(src: src, useManifest: useManifest) + queryString],'')
} else {
outPw << '<script type="text/javascript" src="' << assetPath(src: src) << queryString << '" ' << paramsToHtmlAttr(outputAttrs) << '></script>' << endOfLine
outPw << '<script type="text/javascript" src="' << assetPath(src: src, useManifest: useManifest) << queryString << '" ' << paramsToHtmlAttr(outputAttrs) << '></script>' << endOfLine
}

}
Expand All @@ -40,50 +41,74 @@ class AssetsTagLib {
*
* @attr href OPTIONAL standard URL attribute
* @attr src OPTIONAL alternate URL attribute, only used if {@code href} isn't supplied, or if {@code href} is Groovy false
* @attr uniq OPTIONAL Output the stylesheet tag for the resource only once per request, note that uniq mode cannot be bundled
*/
def stylesheet = {final attrs ->
final GrailsPrintWriter outPw = out
element(attrs, 'css', 'text/css', Objects.toString(attrs.remove('href'), null)) {final String src, final String queryString, final outputAttrs, final String endOfLine ->
element(attrs, 'css', 'text/css', Objects.toString(attrs.remove('href'), null)) {final String src, final String queryString, final outputAttrs, final String endOfLine, final boolean useManifest ->
outPw << '<link rel="stylesheet" href="' << assetPath(src: src, useManifest: useManifest) << queryString << '" ' << paramsToHtmlAttr(outputAttrs) << '/>'
if (endOfLine) {
outPw << '<link rel="stylesheet" href="' << assetPath(src: src) << queryString << '" ' << paramsToHtmlAttr(outputAttrs) << '/>' << endOfLine
}
else {
outPw << link([rel: 'stylesheet', href: src] + outputAttrs)
outPw << endOfLine
}
}
}

private boolean isIncluded(def path) {
HashSet<String> memo = request."$ASSET_REQUEST_MEMO"
if (memo == null) {
memo = new HashSet<String>()
request."$ASSET_REQUEST_MEMO" = memo
}
!memo.add(path)
}

private static def nameAndExtension(String src, String ext) {
int lastDotIndex = src.lastIndexOf('.')
if (lastDotIndex >= 0) {
[uri: src.substring(0, lastDotIndex), extension: src.substring(lastDotIndex + 1)]
} else {
[uri: src, extension: ext]
}
}

private void element(final attrs, final String ext, final String contentType, final String srcOverride, final Closure<GrailsPrintWriter> output) {
def src = attrs.remove('src')
if (srcOverride) {
src = srcOverride
}
def uniqMode = attrs.remove('uniq') != null

src = "${AssetHelper.nameWithoutExtension(src)}.${ext}"
def conf = grailsApplication.config.grails.assets

final def nonBundledMode = (!AssetPipelineConfigHolder.manifest && conf.bundle != true && attrs.remove('bundle') != 'true')
final def nonBundledMode = uniqMode || (!AssetPipelineConfigHolder.manifest && conf.bundle != true && attrs.remove('bundle') != 'true')

if (! nonBundledMode) {
output(src, '', attrs, '')
output(src, '', attrs, '', true)
}
else {
final int lastDotIndex = src.lastIndexOf('.')
final def uri
final def extension
if (lastDotIndex >= 0) {
uri = src.substring(0, lastDotIndex)
extension = src.substring(lastDotIndex + 1)
}
else {
uri = src
extension = ext
}
def name = nameAndExtension(src, ext)
final String uri = name.uri
final String extension = name.extension

final String queryString =
attrs.charset \
? "?compile=false&encoding=${attrs.charset}"
: '?compile=false'
if (uniqMode && isIncluded(name)) {
return
}
def useManifest = !nonBundledMode

AssetPipeline.getDependencyList(uri, contentType, extension)?.each {
output(it.path, queryString, attrs, LINE_BREAK)
if (uniqMode) {
def path = nameAndExtension(it.path, ext)
if (path.uri == uri || !isIncluded(path)) {
output(it.path, queryString, attrs, LINE_BREAK, useManifest)
}
} else {
output(it.path, queryString, attrs, LINE_BREAK, useManifest)
}
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineGrailsPlugin.groovy
View file
Expand Up @@ -89,7 +89,10 @@ class AssetPipelineGrailsPlugin extends grails.plugins.Plugin {
log.warn "Unable to find asset-pipeline manifest, etags will not be properly generated"
}
}
if(manifestFile?.exists()) {

def useManifest = assetsConfig.useManifest ?: true

if(useManifest && manifestFile?.exists()) {
try {
manifestProps.load(manifestFile.inputStream)
assetsConfig.manifest = manifestProps
Expand Down
11 changes: 9 additions & 2 deletions asset-pipeline-grails/src/main/groovy/asset/pipeline/grails/AssetProcessorService.groovy
View file
Expand Up @@ -46,10 +46,17 @@ class AssetProcessorService {
return mapping
}

String getAssetPath(final String path, final boolean useManifest) {
getAssetPath(path, grailsApplication.config.grails.assets as ConfigObject, useManifest)
}

String getAssetPath(final String path, final ConfigObject conf = grailsApplication.config.grails.assets) {
String getAssetPath(final String path, final ConfigObject conf = grailsApplication.config.grails.assets, final boolean useManifest = true) {
final String relativePath = trimLeadingSlash(path)
return manifest?.getProperty(relativePath) ?: relativePath
if (useManifest) {
return manifest?.getProperty(relativePath) ?: relativePath
} else {
return relativePath
}
}


Expand Down
41 changes: 36 additions & 5 deletions asset-pipeline-grails/src/test/groovy/asset/pipeline/grails/AssetsTagLibSpec.groovy
View file
Expand Up @@ -15,14 +15,11 @@
*/
package asset.pipeline.grails


import asset.pipeline.AssetPipelineConfigHolder
import asset.pipeline.fs.FileSystemAssetResolver
import grails.test.mixin.TestFor
import spock.lang.Specification



/**
* @author David Estes
*/
Expand Down Expand Up @@ -89,12 +86,34 @@ class AssetsTagLibSpec extends Specification {
output == '<script type="text/javascript" src="/assets/asset-pipeline/test/test.js?compile=false" ></script>' + LINE_BREAK + '<script type="text/javascript" src="/assets/asset-pipeline/test/libs/file_a.js?compile=false" ></script>' + LINE_BREAK + '<script type="text/javascript" src="/assets/asset-pipeline/test/libs/file_c.js?compile=false" ></script>' + LINE_BREAK + '<script type="text/javascript" src="/assets/asset-pipeline/test/libs/file_b.js?compile=false" ></script>' + LINE_BREAK + '<script type="text/javascript" src="/assets/asset-pipeline/test/libs/subset/subset_a.js?compile=false" ></script>' + LINE_BREAK
}

void "should not return javascript link twice in uniq mode"() {
given:
final def assetSrc = "asset-pipeline/test/test_simple_require.js"
final def depAssetSrc = "asset-pipeline/test/libs/file_a.js"
expect:
tagLib.javascript(src: assetSrc, uniq: true) == '<script type="text/javascript" src="/assets/asset-pipeline/test/libs/file_a.js?compile=false" ></script>' + LINE_BREAK + '<script type="text/javascript" src="/assets/asset-pipeline/test/test_simple_require.js?compile=false" ></script>' + LINE_BREAK
tagLib.javascript(src: assetSrc, uniq: true) == ''
tagLib.javascript(src: depAssetSrc, uniq: true) == ''
cleanup:
request."${AssetsTagLib.ASSET_REQUEST_MEMO}" = null
}

void "should return javascript and stylesheets of the same basename"() {
given:
final def jsAssetSrc = "asset-pipeline/test/test.js"
final def cssAssetSrc = "asset-pipeline/test/test.css"
expect:
tagLib.javascript(src: jsAssetSrc, uniq: true) != ''
tagLib.javascript(src: jsAssetSrc, uniq: true) == ''
tagLib.stylesheet(src: cssAssetSrc, uniq: true) != ''
}

void "should return stylesheet link tag when debugMode is off"() {
given:
grailsApplication.config.grails.assets.bundle = true
final def assetSrc = "asset-pipeline/test/test.css"
expect:
tagLib.stylesheet(href: assetSrc) == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css"/>'
tagLib.stylesheet(href: assetSrc) == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css" />'
}

void "should always return stylesheet link tag when bundle attr is 'true'"() {
Expand All @@ -104,7 +123,7 @@ class AssetsTagLibSpec extends Specification {
params."_debugAssets" = "y"
final def assetSrc = "asset-pipeline/test/test.css"
expect:
tagLib.stylesheet(href: assetSrc, bundle: 'true') == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css"/>'
tagLib.stylesheet(href: assetSrc, bundle: 'true') == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css" />'
}

void "should return stylesheet link tag with seperated files when debugMode is on"() {
Expand All @@ -121,6 +140,18 @@ class AssetsTagLibSpec extends Specification {
output == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css?compile=false" />' + LINE_BREAK + '<link rel="stylesheet" href="/assets/asset-pipeline/test/test2.css?compile=false" />' + LINE_BREAK
}

void "should not return stylesheet link twice in uniq mode"() {
given:
final def assetSrc = "asset-pipeline/test/test.css"
final def depAssetSrc = "asset-pipeline/test/test2.css"
expect:
tagLib.stylesheet(src: assetSrc, uniq: true) == '<link rel="stylesheet" href="/assets/asset-pipeline/test/test.css?compile=false" />' + LINE_BREAK + '<link rel="stylesheet" href="/assets/asset-pipeline/test/test2.css?compile=false" />' + LINE_BREAK
tagLib.stylesheet(src: depAssetSrc, uniq: true) == ''
tagLib.stylesheet(src: assetSrc, uniq: true) == ''
cleanup:
request."${AssetsTagLib.ASSET_REQUEST_MEMO}" = null
}

void "should return image tag"() {
given:
final def assetSrc = "grails_logo.png"
Expand Down

0 comments on commit f8bd3d7

Please sign in to comment.