From 9d89e41d77d22d93f9b74b7b3fcc04038000835d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Glawaty?= <tomasglawaty@icloud.com>
Date: Thu, 29 Aug 2024 02:54:13 +0200
Subject: [PATCH] Loading options as expressions

- the option `loading-offset` is now deprecated
- the option `loading` is now processed as expression, same as the `fetchpriority` option
- options from the AMP application have higher priority than options from the client definitions
- added method `Options.evaluate()`
---
 src/banner/banner.mjs                 |  4 +--
 src/banner/managed/managed-banner.mjs |  2 +-
 src/banner/options.mjs                | 43 +++++++++++++++++++++++++--
 src/client/standard/client.mjs        |  2 +-
 src/renderer/banner-renderer.mjs      |  2 --
 src/template/multiple.mjs             |  5 ++--
 src/template/random.mjs               |  5 ++--
 src/template/single.mjs               |  5 ++--
 8 files changed, 50 insertions(+), 18 deletions(-)

diff --git a/src/banner/banner.mjs b/src/banner/banner.mjs
index a046671..4a06e33 100644
--- a/src/banner/banner.mjs
+++ b/src/banner/banner.mjs
@@ -95,8 +95,8 @@ export class Banner {
         return null;
     }
 
-    mergeOptions(options) {
-        this.#options.merge(options);
+    overrideOptions(options) {
+        this.#options.override(options);
     }
 
     redrawIfNeeded() {
diff --git a/src/banner/managed/managed-banner.mjs b/src/banner/managed/managed-banner.mjs
index 1e65628..d192d4f 100644
--- a/src/banner/managed/managed-banner.mjs
+++ b/src/banner/managed/managed-banner.mjs
@@ -164,7 +164,7 @@ export class ManagedBanner extends Banner {
         });
 
         if ('options' in responseData) {
-            this.mergeOptions(responseData.options);
+            this.overrideOptions(responseData.options);
         }
 
         const banners = [];
diff --git a/src/banner/options.mjs b/src/banner/options.mjs
index 301532a..6314c3a 100644
--- a/src/banner/options.mjs
+++ b/src/banner/options.mjs
@@ -1,6 +1,9 @@
+import { evaluateExpression } from '../utils/expression.mjs';
+
 export class Options {
     constructor(options) {
-        this.options = options;
+        this.options = {};
+        this.options = this.#fixOptionsCompatibility({...options});
     }
 
     has(optionName) {
@@ -11,7 +14,41 @@ export class Options {
         return this.options[optionName] || defaultValue;
     }
 
-    merge(options) {
-        this.options = {...this.options, ...options};
+    evaluate(optionName, index) {
+        if (undefined === this.options[optionName]) {
+            return null;
+        }
+
+        return evaluateExpression(
+            this.options[optionName],
+            index
+        );
+    }
+
+    override(options) {
+        this.options = {
+            ...this.options,
+            ...this.#fixOptionsCompatibility({...options}),
+        };
+    }
+
+    #fixOptionsCompatibility(options) {
+        if ('loading-offset' in options) {
+            const offset = options['loading-offset'];
+            const loading = 'loading' in options ? options['loading'] : ('loading' in this.options ? this.options['loading'] : null);
+            const loadingExpression = null !== loading ? `>=${offset}:${loading}` : null;
+            let message = `AMP deprecation warning: The banner option "loading-offset" is deprecated and will be removed in some future release.`;
+
+            if (null !== loadingExpression) {
+                message += ` Instead of options {"loading": "${loading}", "loading-offset": "${offset}"} use an expression based option {"loading": "${loadingExpression}"}.`;
+            }
+
+            console.warn(message);
+
+            delete options['loading-offset'];
+            null !== loadingExpression && (options.loading = loadingExpression);
+        }
+
+        return options;
     }
 }
diff --git a/src/client/standard/client.mjs b/src/client/standard/client.mjs
index 07c288b..bfaab07 100644
--- a/src/client/standard/client.mjs
+++ b/src/client/standard/client.mjs
@@ -228,7 +228,7 @@ export class Client {
 
                 if ('embed' === positionData.mode) {
                     if ('options' in positionData) {
-                        banner.mergeOptions(positionData.options);
+                        banner.overrideOptions(positionData.options);
                     }
 
                     this.createBanner(banner.element, banner.position, banner.rawResources, banner.options.options, positionData.mode);
diff --git a/src/renderer/banner-renderer.mjs b/src/renderer/banner-renderer.mjs
index 0c58c28..712f60c 100644
--- a/src/renderer/banner-renderer.mjs
+++ b/src/renderer/banner-renderer.mjs
@@ -1,5 +1,4 @@
 import { TemplateLoader } from './temnplate-loader.mjs';
-import { evaluateExpression } from '../utils/expression.mjs';
 
 export class BannerRenderer {
     #loader;
@@ -15,7 +14,6 @@ export class BannerRenderer {
         return this.#loader.getTemplate(banner.positionData.displayType)({
             banner: banner,
             data: banner.bannerData,
-            expr: evaluateExpression,
         });
     }
 }
diff --git a/src/template/multiple.mjs b/src/template/multiple.mjs
index a242aef..708c1ae 100644
--- a/src/template/multiple.mjs
+++ b/src/template/multiple.mjs
@@ -1,7 +1,6 @@
 /**
  * {ManagedBanner} banner
  * {Array<BannerData>} data
- * {Function(String expression, Integer index): String|null} expr
  */
 export default `
     <div class="amp-banner amp-banner--multiple">
@@ -27,8 +26,8 @@ export default `
                                      <% if(null !== banner.positionData.dimensions.width) { %>width="<%- banner.positionData.dimensions.width %>"<% } %>
                                      <% if(null !== banner.positionData.dimensions.height) { %>height="<%- banner.positionData.dimensions.height %>"<% } %>
                                      <% if('' !== b.content.title) { %>title="<%- b.content.title %>"<% } %>
-                                     <% if(banner.options.has('loading') && index >= parseInt(banner.options.get('loading-offset', 0))) { %>loading="<%- banner.options.get('loading') %>"<% } %>
-                                     <% if(banner.options.has('fetchpriority') && expr(banner.options.get('fetchpriority'), index)) { %>fetchpriority="<%- expr(banner.options.get('fetchpriority'), index) %>"<% } %>>
+                                     <% var loading; if(loading = banner.options.evaluate('loading', index)) { %>loading="<%- loading %>"<% } %>
+                                     <% var fetchPriority; if(fetchPriority = banner.options.evaluate('fetchpriority', index)) { %>fetchpriority="<%- fetchPriority %>"<% } %>>
                             </picture>
                         </a>
                     <% } else if ('html' === b.content.type) { %>
diff --git a/src/template/random.mjs b/src/template/random.mjs
index 290be90..8936cc5 100644
--- a/src/template/random.mjs
+++ b/src/template/random.mjs
@@ -1,7 +1,6 @@
 /**
  * {ManagedBanner} banner
  * {BannerData} data
- * {Function(String expression, Integer index): String|null} expr
  */
 export default `
     <div class="amp-banner amp-banner--random"
@@ -24,8 +23,8 @@ export default `
                          <% if(null !== banner.positionData.dimensions.width) { %>width="<%- banner.positionData.dimensions.width %>"<% } %>
                          <% if(null !== banner.positionData.dimensions.height) { %>height="<%- banner.positionData.dimensions.height %>"<% } %>
                          <% if('' !== data.content.title) { %>title="<%- data.content.title %>"<% } %>
-                         <% if(banner.options.has('loading')) { %>loading="<%- banner.options.get('loading') %>"<% } %>
-                         <% if(banner.options.has('fetchpriority') && expr(banner.options.get('fetchpriority'), 0)) { %>fetchpriority="<%- expr(banner.options.get('fetchpriority'), 0) %>"<% } %>>
+                         <% var loading; if(loading = banner.options.evaluate('loading', 0)) { %>loading="<%- loading %>"<% } %>
+                         <% var fetchPriority; if(fetchPriority = banner.options.evaluate('fetchpriority', 0)) { %>fetchpriority="<%- fetchPriority %>"<% } %>>
                 </picture>
             </a>
         <% } else if ('html' === data.content.type) { %>
diff --git a/src/template/single.mjs b/src/template/single.mjs
index 5938484..4047d03 100644
--- a/src/template/single.mjs
+++ b/src/template/single.mjs
@@ -1,7 +1,6 @@
 /**
  * {ManagedBanner} banner
  * {BannerData} data
- * {Function(String expression, Integer index): String|null} expr
  */
 export default `
     <div class="amp-banner amp-banner--single"
@@ -24,8 +23,8 @@ export default `
                          <% if(null !== banner.positionData.dimensions.width) { %>width="<%- banner.positionData.dimensions.width %>"<% } %>
                          <% if(null !== banner.positionData.dimensions.height) { %>height="<%- banner.positionData.dimensions.height %>"<% } %>
                          <% if('' !== data.content.title) { %>title="<%- data.content.title %>"<% } %>
-                         <% if(banner.options.has('loading')) { %>loading="<%- banner.options.get('loading') %>"<% } %>
-                         <% if(banner.options.has('fetchpriority') && expr(banner.options.get('fetchpriority'), 0)) { %>fetchpriority="<%- expr(banner.options.get('fetchpriority'), 0) %>"<% } %>>
+                         <% var loading; if(loading = banner.options.evaluate('loading', 0)) { %>loading="<%- loading %>"<% } %>
+                         <% var fetchPriority; if(fetchPriority = banner.options.evaluate('fetchpriority', 0)) { %>fetchpriority="<%- fetchPriority %>"<% } %>>
                 </picture>
             </a>
         <% } else if ('html' === data.content.type) { %>