From d0635078f2a5fe4e51f963812522dc92ac2486b5 Mon Sep 17 00:00:00 2001
From: caalador <mikael.grankvist@gmail.com>
Date: Tue, 17 Nov 2020 15:31:32 +0200
Subject: [PATCH] feat: Only use one theme and fail for duplicates (#9406)

Now we only handle the theme with the name inside the Theme annotation.

Fixes #9383
# Conflicts:
#	flow-tests/test-themes/pom.xml
---
 .../frontend/TaskUpdateThemeImport.java       |   9 +-
 .../application-theme-plugin.js               | 103 ++++++++++--------
 .../application-theme-plugin/package.json     |   2 +-
 .../src/main/resources/webpack.generated.js   |   2 +-
 .../frontend/theme/no-copy/no-copy.txt        |   2 +
 flow-tests/test-themes/pom.xml                |  22 +++-
 .../vaadin/flow/uitest/ui/theme/ThemeIT.java  |  18 ++-
 7 files changed, 96 insertions(+), 62 deletions(-)
 create mode 100644 flow-tests/test-themes/frontend/theme/no-copy/no-copy.txt

diff --git a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateThemeImport.java b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateThemeImport.java
index b35de020581..db9e874c505 100644
--- a/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateThemeImport.java
+++ b/flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateThemeImport.java
@@ -57,11 +57,10 @@ public void execute() throws ExecutionFailedException {
         }
 
         try {
-            FileUtils.write(themeImportFile,
-                "import {applyTheme as _applyTheme} from 'theme/" + theme
-                    .getName() + "/" + theme.getName()
-                    + ".js';\nexport const applyTheme = _applyTheme;\n",
-                StandardCharsets.UTF_8);
+            FileUtils.write(themeImportFile, String.format(
+                "import {applyTheme as _applyTheme} from 'theme/%s/%s.js';%n"
+                    + "export const applyTheme = _applyTheme;%n",
+                theme.getName(), theme.getName()), StandardCharsets.UTF_8);
         } catch (IOException e) {
             System.out.println("Throwing exception " + e.getMessage());
             throw new ExecutionFailedException(
diff --git a/flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js b/flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js
index ac06fd2f931..167df800ec6 100644
--- a/flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js
+++ b/flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js
@@ -21,14 +21,15 @@ const copyThemeResources = require('./theme-copy');
 
 let logger;
 
+// matches theme folder name in 'theme/my-theme/my-theme.js'
+const nameRegex = /theme\/(.*)\/\1.js/g;
+
 /**
  * The application theme plugin is for generating, collecting and copying of theme files for the application theme.
  *
- * TODO: enable giving themes to handle #9383
- *
  * The plugin should be supplied with the paths for
  *
- *  themeJarFolder                  - theme folder inside a jar
+ *  themeResourceFolder             - theme folder where flow copies local and jar resource frontend files
  *  themeProjectFolders             - array of possible locations for theme folders inside the project
  *  projectStaticAssetsOutputFolder - path to where static assets should be put
  */
@@ -36,13 +37,13 @@ class ApplicationThemePlugin {
   constructor(options) {
     this.options = options;
 
-    if(!this.options.themeJarFolder) {
-      throw new Error("Missing themeJarFolder path");
+    if (!this.options.themeResourceFolder) {
+      throw new Error("Missing themeResourceFolder path");
     }
-    if(!this.options.projectStaticAssetsOutputFolder) {
+    if (!this.options.projectStaticAssetsOutputFolder) {
       throw new Error("Missing projectStaticAssetsOutputFolder path");
     }
-    if(!this.options.themeProjectFolders) {
+    if (!this.options.themeProjectFolders) {
       throw new Error("Missing themeProjectFolders path array");
     }
   }
@@ -51,17 +52,44 @@ class ApplicationThemePlugin {
     logger = compiler.getInfrastructureLogger("ApplicationThemePlugin");
 
     compiler.hooks.afterEnvironment.tap("ApplicationThemePlugin", () => {
-      if (fs.existsSync(this.options.themeJarFolder)) {
-        logger.debug("Found themeFolder in jar file ", this.options.themeJarFolder);
-        handleThemes(this.options.themeJarFolder, this.options.projectStaticAssetsOutputFolder);
-      }
+      const generatedThemeFile = path.resolve(this.options.themeResourceFolder, "theme-generated.js");
+      if (fs.existsSync(generatedThemeFile)) {
+
+        // read theme name from the theme-generated.js as there we always mark the used theme for webpack to handle.
+        const themeName = nameRegex.exec(fs.readFileSync(generatedThemeFile, {encoding: 'utf8'}))[1];
+        if (!themeName) {
+          throw new Error("Couldn't parse theme name from '" + generatedThemeFile + "'.");
+        }
 
-      this.options.themeProjectFolders.forEach((themeProjectFolder) => {
-        if (fs.existsSync(themeProjectFolder)) {
-          logger.debug("Found themeFolder from ", themeProjectFolder);
-          handleThemes(themeProjectFolder, this.options.projectStaticAssetsOutputFolder);
+        let themeFound = false;
+        for (let i = 0; i<this.options.themeProjectFolders.length; i++) {
+          const themeProjectFolder = this.options.themeProjectFolders[i];
+          if (fs.existsSync(themeProjectFolder)) {
+            logger.info("Searching theme folder ", themeProjectFolder, " for theme ", themeName);
+            const handled = handleThemes(themeName, themeProjectFolder, this.options.projectStaticAssetsOutputFolder);
+            if (handled) {
+              if(themeFound) {
+                throw new Error("Found theme filed in '" + themeProjectFolder + "' and '"
+                  + themeFound + "'. Theme should only be available in one folder");
+              }
+              logger.info("Found theme files from '", themeProjectFolder, "'");
+              themeFound = themeProjectFolder;
+            }
+          }
         }
-      });
+
+        if (fs.existsSync(this.options.themeResourceFolder)) {
+          if (themeFound && fs.existsSync(path.resolve(this.options.themeResourceFolder, themeName))) {
+            throw new Error("Theme '" + themeName + "'should not exist inside a jar and in the project at the same time\n" +
+              "Extending another theme is possible by adding { \"parent\": \"my-parent-theme\" } entry to the theme.json file inside your theme folder.");
+          }
+          logger.debug("Searching theme jar resource folder ", this.options.themeResourceFolder, " for theme ", themeName);
+          handleThemes(themeName, this.options.themeResourceFolder, this.options.projectStaticAssetsOutputFolder);
+        }
+      } else {
+        logger.debug("Skipping Vaadin application theme handling.");
+        logger.trace("Most likely no @Theme annotation for application or only themeClass used.");
+      }
     });
   }
 }
@@ -71,44 +99,23 @@ module.exports = ApplicationThemePlugin;
 /**
  * Copies static resources for theme and generates/writes the [theme-name].js for webpack to handle.
  *
- * @param {path} themesFolder folder containing application theme folders
- * @param {path} projectStaticAssetsOutputFolder folder to output files to
+ * @param {string} themeName name of theme to handle
+ * @param {string} themesFolder folder containing application theme folders
+ * @param {string} projectStaticAssetsOutputFolder folder to output files to
+ *
+ * @returns true if theme was found else false.
  */
-function handleThemes(themesFolder, projectStaticAssetsOutputFolder) {
-  const dir = getThemeFoldersSync(themesFolder);
-  logger.debug("Found", dir.length, "theme directories");
-
-  for (let i = 0; i < dir.length; i++) {
-    const folder = dir[i];
-
-    const themeName = folder;
-    const themeFolder = path.resolve(themesFolder, themeName);
+function handleThemes(themeName, themesFolder, projectStaticAssetsOutputFolder) {
+  const themeFolder = path.resolve(themesFolder, themeName);
+  if (fs.existsSync(themeFolder)) {
     logger.debug("Found theme ", themeName, " in folder ", themeFolder);
 
     copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder);
 
-    const themeFile = generateThemeFile(
-      themeFolder,
-      themeName
-    );
+    const themeFile = generateThemeFile(themeFolder, themeName);
 
     fs.writeFileSync(path.resolve(themeFolder, themeName + '.js'), themeFile);
+    return true;
   }
+  return false;
 };
-
-/**
- * Collect all folders under the given theme project folder.
- * The found sub folders are the actual theme 'implementations'
- *
- * @param {string | Buffer | URL} folder theme project folder to collect folders from
- * @returns {string[]} array containing found folder names
- */
-function getThemeFoldersSync(folder) {
-  const themeFolders = [];
-  fs.readdirSync(folder).forEach(file => {
-    if (fs.statSync(path.resolve(folder, file)).isDirectory()) {
-      themeFolders.push(file);
-    }
-  });
-  return themeFolders;
-}
diff --git a/flow-server/src/main/resources/plugins/application-theme-plugin/package.json b/flow-server/src/main/resources/plugins/application-theme-plugin/package.json
index 31eaccbe1b7..b6c064ed388 100644
--- a/flow-server/src/main/resources/plugins/application-theme-plugin/package.json
+++ b/flow-server/src/main/resources/plugins/application-theme-plugin/package.json
@@ -5,7 +5,7 @@
   ],
   "repository": "vaadin/flow",
   "name": "@vaadin/application-theme-plugin",
-  "version": "0.1.0",
+  "version": "0.1.1",
   "main": "application-theme-plugin.js",
   "author": "Vaadin Ltd",
   "license": "Apache-2.0",
diff --git a/flow-server/src/main/resources/webpack.generated.js b/flow-server/src/main/resources/webpack.generated.js
index f8cdbd1a3b1..4dc2a3522dd 100644
--- a/flow-server/src/main/resources/webpack.generated.js
+++ b/flow-server/src/main/resources/webpack.generated.js
@@ -213,7 +213,7 @@ module.exports = {
     })] : []),
 
     new ApplicationThemePlugin({
-      themeJarFolder: path.resolve(flowFrontendFolder, 'theme'),
+      themeResourceFolder: path.resolve(flowFrontendFolder, 'theme'),
       themeProjectFolders: themeProjectFolders,
       projectStaticAssetsOutputFolder: projectStaticAssetsOutputFolder,
     }),
diff --git a/flow-tests/test-themes/frontend/theme/no-copy/no-copy.txt b/flow-tests/test-themes/frontend/theme/no-copy/no-copy.txt
new file mode 100644
index 00000000000..19b79daf2a5
--- /dev/null
+++ b/flow-tests/test-themes/frontend/theme/no-copy/no-copy.txt
@@ -0,0 +1,2 @@
+This text file should not get copied
+as only app-theme should be used in module.
diff --git a/flow-tests/test-themes/pom.xml b/flow-tests/test-themes/pom.xml
index 1ae1d289075..c84f10faf74 100644
--- a/flow-tests/test-themes/pom.xml
+++ b/flow-tests/test-themes/pom.xml
@@ -55,10 +55,24 @@
             <plugin>
                 <groupId>org.eclipse.jetty</groupId>
                 <artifactId>jetty-maven-plugin</artifactId>
-<!--                <configuration>-->
-<!--                    &lt;!&ndash; Use war output directory instead of default src/main/webapp to get the webpack bundles  &ndash;&gt;-->
-<!--                    <webAppSourceDirectory>${project.build.directory}/${project.build.finalName}</webAppSourceDirectory>-->
-<!--                </configuration>-->
+                <!-- do not allow faulty app shell annotations for module -->
+                <!-- This should be removed when flow-tests do not break app shell annotation-->
+                <!-- flow-tests should not allow faulty annotations -->
+                <configuration>
+                    <!-- This is needed as Jetty by default only serves 'src/main/webapp' -->
+                    <!-- and not the servlet 3.0 spec for JAR when not executing a JAR -->
+                    <webApp>
+                        <resourceBases>
+                            <resourceBase>${project.build.outputDirectory}/META-INF/resources</resourceBase>
+                        </resourceBases>
+                    </webApp>
+                    <systemProperties>
+                        <systemProperty>
+                            <name>vaadin.allow.appshell.annotations</name>
+                            <value>false</value>
+                        </systemProperty>
+                    </systemProperties>
+                </configuration>
             </plugin>
         </plugins>
     </build>
diff --git a/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java b/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java
index 513b4b01dab..5e9055a83c0 100644
--- a/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java
+++ b/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java
@@ -15,17 +15,25 @@
  */
 package com.vaadin.flow.uitest.ui.theme;
 
-import java.util.List;
-
 import org.junit.Assert;
 import org.junit.Test;
 import org.openqa.selenium.By;
 
 import com.vaadin.flow.testutil.ChromeBrowserTest;
-import com.vaadin.testbench.TestBenchElement;
 
 public class ThemeIT extends ChromeBrowserTest {
 
+    @Test
+    public void secondTheme_staticFilesNotCopied() {
+        getDriver().get(getRootURL() + "/theme/app-theme/img/bg.jpg");
+        Assert.assertFalse("app-theme static files should be copied",
+            driver.getPageSource().contains("Could not navigate"));
+
+        getDriver().get(getRootURL() + "/theme/no-copy/no-copy.txt");
+        Assert.assertTrue("no-copy theme should not be handled",
+            driver.getPageSource().contains("Could not navigate to 'theme/no-copy/no-copy.txt'"));
+    }
+
     @Test
     public void applicationTheme_GlobalCss_isUsed() {
         open();
@@ -35,6 +43,10 @@ public void applicationTheme_GlobalCss_isUsed() {
         Assert.assertEquals(
             "url(\"" + getRootURL() + "/theme/app-theme/img/bg.jpg\")",
             findElement(By.tagName("body")).getCssValue("background-image"));
+
+        getDriver().get(getRootURL() + "/theme/app-theme/img/bg.jpg");
+        Assert.assertFalse("app-theme background file should be served",
+            driver.getPageSource().contains("Could not navigate"));
     }
 
     @Override