Fix moveAssetsInOrder transform: file:///private/tmp/static/font.eot …

…already exists in the graph, cannot update url Reported by @salomvary on gitter
assetgraph · Mar 4, 2018 · 879b4a1 · 879b4a1
1 parent e21dda6
commit 879b4a1
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 4 deletions.
diff --git a/lib/transforms/buildProduction.js b/lib/transforms/buildProduction.js
@@ -396,13 +396,13 @@ module.exports = function (options) {
 
         if (!options.noFileRev) {
             await assetGraph.moveAssetsInOrder(moveAssetsInOrderQuery, (asset, assetGraph) => {
-                let targetUrl = '/static/';
+                let baseUrl = (assetGraph.canonicalRoot || assetGraph.root) + 'static/';
                 // Conservatively assume that all JavaScriptStaticUrl relations pointing at non-images are intended to be fetched via XHR
                 // and thus cannot be put on a CDN because of same origin restrictions:
                 const hasIncomingJavaScriptStaticUrlOrServiceWorkerRelations = assetGraph.findRelations({to: asset, type: {$in: ['JavaScriptStaticUrl', 'JavaScriptServiceWorkerRegistration', 'HtmlServiceWorkerRegistration']}}).length > 0;
                 if (options.cdnRoot && asset.type !== 'Htc' && asset.extension !== '.jar' && (asset.type !== 'Html' || options.cdnHtml) && (asset.isImage || !hasIncomingJavaScriptStaticUrlOrServiceWorkerRelations) || (options.cdnRoot && options.cdnFlash && asset.type === 'Flash')) {
-                    targetUrl = options.cdnRoot;
-                    assetGraph.findRelations({to: asset}).forEach(function (incomingRelation) {
+                    baseUrl = options.cdnRoot;
+                    assetGraph.findRelations({ to: asset }).forEach(function (incomingRelation) {
                         if (/^\/\//.test(options.cdnRoot)) {
                             incomingRelation.hrefType = 'protocolRelative';
                         } else if ((asset.type === 'SourceMap' || hasIncomingJavaScriptStaticUrlOrServiceWorkerRelations) && /^https?:/.test(options.cdnRoot)) {
@@ -418,7 +418,15 @@ module.exports = function (options) {
                         }
                     });
                 }
-                return targetUrl + asset.fileName;
+                // Find a way to avoid repeating this construct over and over.
+                // Maybe the _urlIndex thing is too strict.
+                let targetUrl = baseUrl + asset.fileName;
+                let nextSuffixToTry = 1;
+                while (assetGraph._urlIndex[targetUrl]) {
+                    targetUrl = `${baseUrl}${asset.baseName}-${nextSuffixToTry}${asset.extension}`;
+                    nextSuffixToTry += 1;
+                }
+                return targetUrl;
             });
             await assetGraph.moveAssetsInOrder(moveAssetsInOrderQuery, function (asset, assetGraph) {
                 return asset.fileName.replace(/\.[^.]+$/, '') + '.' + asset.md5Hex.substr(0, 10) + asset.extension + asset.url.replace(/^[^#\?]*(?:)/, ''); // Preserve query string and fragment identifier

diff --git a/test/transforms/buildProduction.js b/test/transforms/buildProduction.js
@@ -1566,4 +1566,15 @@ describe('buildProduction', function () {
                 .and('not to contain', '.toString(\'url\')');
         });
     });
+
+    // Turning on deduplication for fonts via mergeIdenticalAssets would require changes here:
+    it('should handle a case with an ?#iefix hack', async function () {
+        const assetGraph = new AssetGraph({root: __dirname + '/../../testdata/transforms/buildProduction/fontWithIeFix/'});
+        await assetGraph.loadAssets('index.html');
+        await assetGraph.buildProduction();
+
+        expect(assetGraph, 'to contain assets', 4);
+        expect(assetGraph, 'to contain asset', {path: '/static/', fileName: 'font.0c064252fc.eot'})
+            .and('to contain asset', {path: '/static/', fileName: 'font.0c064252fc.eot'});
+    });
 });
diff --git a/testdata/transforms/buildProduction/fontWithIeFix/font.eot b/testdata/transforms/buildProduction/fontWithIeFix/font.eot
diff --git a/testdata/transforms/buildProduction/fontWithIeFix/index.html b/testdata/transforms/buildProduction/fontWithIeFix/index.html
@@ -0,0 +1,22 @@
+<!doctype html>
+
+<head>
+    <style>
+        @font-face {
+            font-family: 'font';
+            src: url('font.eot');
+            font-weight: 100;
+            font-style: normal;
+        }
+
+        @font-face {
+            font-family: 'font-thin';
+            src: url('font.eot');
+            src: local('font thin'), local('font-thin'), url('font.eot?#iefix');
+        }
+    </style>
+
+</head>
+
+<body>
+</body>