From f8bd3d701a5fe14a30f5d89c6e8796f590d4643b Mon Sep 17 00:00:00 2001 From: Matt Sullivan Date: Wed, 23 May 2018 15:35:41 -0700 Subject: [PATCH] Add feature for deduplicating javascript and stylesheets (#205) * 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 --- .../test/test_simple_require.js | 3 + .../pipeline/grails/AssetMethodTagLib.groovy | 13 +-- .../asset/pipeline/grails/AssetsTagLib.groovy | 79 ++++++++++++------- .../pipeline/AssetPipelineGrailsPlugin.groovy | 5 +- .../grails/AssetProcessorService.groovy | 11 ++- .../pipeline/grails/AssetsTagLibSpec.groovy | 41 ++++++++-- 6 files changed, 112 insertions(+), 40 deletions(-) create mode 100644 asset-pipeline-grails/grails-app/assets/javascripts/asset-pipeline/test/test_simple_require.js diff --git a/asset-pipeline-grails/grails-app/assets/javascripts/asset-pipeline/test/test_simple_require.js b/asset-pipeline-grails/grails-app/assets/javascripts/asset-pipeline/test/test_simple_require.js new file mode 100644 index 00000000..b769ba26 --- /dev/null +++ b/asset-pipeline-grails/grails-app/assets/javascripts/asset-pipeline/test/test_simple_require.js @@ -0,0 +1,3 @@ +//=require libs/file_a + +console.log("This and file_a should be included"); \ No newline at end of file diff --git a/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetMethodTagLib.groovy b/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetMethodTagLib.groovy index e2a30b94..348a5b1b 100644 --- a/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetMethodTagLib.groovy +++ b/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetMethodTagLib.groovy @@ -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) } } diff --git a/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetsTagLib.groovy b/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetsTagLib.groovy index e440dcf6..c1f9629b 100644 --- a/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetsTagLib.groovy +++ b/asset-pipeline-grails/grails-app/taglib/asset/pipeline/grails/AssetsTagLib.groovy @@ -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 << '' << endOfLine + outPw << '' << endOfLine } } @@ -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 << '' if (endOfLine) { - outPw << '' << endOfLine - } - else { - outPw << link([rel: 'stylesheet', href: src] + outputAttrs) + outPw << endOfLine } } } + private boolean isIncluded(def path) { + HashSet memo = request."$ASSET_REQUEST_MEMO" + if (memo == null) { + memo = new HashSet() + 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 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) + } } } } diff --git a/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineGrailsPlugin.groovy b/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineGrailsPlugin.groovy index 103e4f26..d8408f38 100644 --- a/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineGrailsPlugin.groovy +++ b/asset-pipeline-grails/src/main/groovy/asset/pipeline/AssetPipelineGrailsPlugin.groovy @@ -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 diff --git a/asset-pipeline-grails/src/main/groovy/asset/pipeline/grails/AssetProcessorService.groovy b/asset-pipeline-grails/src/main/groovy/asset/pipeline/grails/AssetProcessorService.groovy index 347def0b..ba114c4a 100644 --- a/asset-pipeline-grails/src/main/groovy/asset/pipeline/grails/AssetProcessorService.groovy +++ b/asset-pipeline-grails/src/main/groovy/asset/pipeline/grails/AssetProcessorService.groovy @@ -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 + } } diff --git a/asset-pipeline-grails/src/test/groovy/asset/pipeline/grails/AssetsTagLibSpec.groovy b/asset-pipeline-grails/src/test/groovy/asset/pipeline/grails/AssetsTagLibSpec.groovy index 918619d5..bb7f72bb 100644 --- a/asset-pipeline-grails/src/test/groovy/asset/pipeline/grails/AssetsTagLibSpec.groovy +++ b/asset-pipeline-grails/src/test/groovy/asset/pipeline/grails/AssetsTagLibSpec.groovy @@ -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 */ @@ -89,12 +86,34 @@ class AssetsTagLibSpec extends Specification { output == '' + LINE_BREAK + '' + LINE_BREAK + '' + LINE_BREAK + '' + LINE_BREAK + '' + 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) == '' + LINE_BREAK + '' + 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) == '' + tagLib.stylesheet(href: assetSrc) == '' } void "should always return stylesheet link tag when bundle attr is 'true'"() { @@ -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') == '' + tagLib.stylesheet(href: assetSrc, bundle: 'true') == '' } void "should return stylesheet link tag with seperated files when debugMode is on"() { @@ -121,6 +140,18 @@ class AssetsTagLibSpec extends Specification { output == '' + LINE_BREAK + '' + 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) == '' + LINE_BREAK + '' + 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"