From c4514f8239faab36bdc79d3c208ffac832c5ef4b Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Mon, 16 Nov 2020 09:05:33 +0200 Subject: [PATCH 1/3] feat: Only handle annotated theme Now we only handle the theme with the name inside the Theme annotation. Fixes #9383 --- .../frontend/TaskUpdateThemeImport.java | 9 +- .../application-theme-plugin.js | 88 ++++++++----------- .../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 | 7 ++ .../uitest/ui/theme/NpmThemedComponentIT.java | 16 ++++ 7 files changed, 70 insertions(+), 56 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 6a75be2efc6..b9396694bbb 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 @@ -58,11 +58,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) { throw new ExecutionFailedException( "Unable to write theme import file", e); 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..e22abe8880f 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 @@ -24,11 +24,9 @@ let logger; /** * 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 +34,13 @@ class ApplicationThemePlugin { constructor(options) { this.options = options; - if(!this.options.themeJarFolder) { + if (!this.options.themeResourceFolder) { throw new Error("Missing themeJarFolder 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 +49,30 @@ 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 nameRegex = /theme\/(.*)\/\1.js/g; // matches theme folder name in 'theme/my-theme/my-theme.js' + 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); + if (fs.existsSync(this.options.themeResourceFolder)) { + logger.debug("Found themeFolder in jar file ", this.options.themeResourceFolder); + handleThemes(themeName, this.options.themeResourceFolder, this.options.projectStaticAssetsOutputFolder); } - }); + + this.options.themeProjectFolders.forEach((themeProjectFolder) => { + if (fs.existsSync(themeProjectFolder)) { + logger.debug("Found themeFolder from ", themeProjectFolder); + handleThemes(themeName, themeProjectFolder, this.options.projectStaticAssetsOutputFolder); + } + }); + } else { + logger.log("No '", generatedThemeFile, "' found. Skipping application theme handling."); + } }); } } @@ -71,44 +82,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 */ -function handleThemes(themesFolder, projectStaticAssetsOutputFolder) { - const dir = getThemeFoldersSync(themesFolder); - logger.debug("Found", dir.length, "theme directories"); +function handleThemes(themeName, themesFolder, projectStaticAssetsOutputFolder) { - for (let i = 0; i < dir.length; i++) { - const folder = dir[i]; - - const themeName = folder; const themeFolder = path.resolve(themesFolder, themeName); - logger.debug("Found theme ", themeName, " in folder ", themeFolder); + if(fs.existsSync(themeFolder)) { + logger.debug("Found theme ", themeName, " in folder ", themeFolder); - copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder); + copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder); - const themeFile = generateThemeFile( - themeFolder, - themeName - ); + const themeFile = generateThemeFile( + themeFolder, + themeName + ); - fs.writeFileSync(path.resolve(themeFolder, themeName + '.js'), themeFile); - } -}; - -/** - * 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); + fs.writeFileSync(path.resolve(themeFolder, themeName + '.js'), themeFile); } - }); - 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 f33486d73cf..d7cba22e14d 100644 --- a/flow-server/src/main/resources/webpack.generated.js +++ b/flow-server/src/main/resources/webpack.generated.js @@ -193,7 +193,7 @@ module.exports = { !devMode && new CompressionPlugin(), 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 7992d8b19fb..863a167b91f 100644 --- a/flow-tests/test-themes/pom.xml +++ b/flow-tests/test-themes/pom.xml @@ -52,6 +52,13 @@ + + + + + ${project.build.outputDirectory}/META-INF/resources + + vaadin.allow.appshell.annotations diff --git a/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/NpmThemedComponentIT.java b/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/NpmThemedComponentIT.java index fe3023906fc..29c21685552 100644 --- a/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/NpmThemedComponentIT.java +++ b/flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/NpmThemedComponentIT.java @@ -26,6 +26,18 @@ public class NpmThemedComponentIT 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"); + System.out.println(" === " + driver.getTitle()); + 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 +47,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")); } @Test From 3830fd5de55fdd889f589e543438e4e554f7b460 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Tue, 17 Nov 2020 11:55:26 +0200 Subject: [PATCH 2/3] Stricter handling on theme duplicates --- .../application-theme-plugin.js | 61 ++++++++++++------- .../vaadin/flow/uitest/ui/theme/ThemeIT.java | 1 - 2 files changed, 39 insertions(+), 23 deletions(-) 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 e22abe8880f..36862fe6129 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,6 +21,9 @@ 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. * @@ -35,7 +38,7 @@ class ApplicationThemePlugin { this.options = options; if (!this.options.themeResourceFolder) { - throw new Error("Missing themeJarFolder path"); + throw new Error("Missing themeResourceFolder path"); } if (!this.options.projectStaticAssetsOutputFolder) { throw new Error("Missing projectStaticAssetsOutputFolder path"); @@ -53,25 +56,39 @@ class ApplicationThemePlugin { 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 nameRegex = /theme\/(.*)\/\1.js/g; // matches theme folder name in 'theme/my-theme/my-theme.js' const themeName = nameRegex.exec(fs.readFileSync(generatedThemeFile, {encoding: 'utf8'}))[1]; if (!themeName) { throw new Error("Couldn't parse theme name from '" + generatedThemeFile + "'."); } - if (fs.existsSync(this.options.themeResourceFolder)) { - logger.debug("Found themeFolder in jar file ", this.options.themeResourceFolder); - handleThemes(themeName, this.options.themeResourceFolder, this.options.projectStaticAssetsOutputFolder); + let themeFound = false; + for (let i = 0; i { - if (fs.existsSync(themeProjectFolder)) { - logger.debug("Found themeFolder from ", themeProjectFolder); - handleThemes(themeName, themeProjectFolder, this.options.projectStaticAssetsOutputFolder); + 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" + + "If extending the jar theme the 'parent' theme feature should be used"); } - }); + logger.debug("Searching theme jar resource folder ", this.options.themeResourceFolder, " for theme ", themeName); + handleThemes(themeName, this.options.themeResourceFolder, this.options.projectStaticAssetsOutputFolder); + } } else { - logger.log("No '", generatedThemeFile, "' found. Skipping application theme handling."); + logger.debug("Skipping Vaadin application theme handling."); + logger.trace("Most likely no @Theme annotation for application or only themeClass used."); } }); } @@ -85,20 +102,20 @@ module.exports = ApplicationThemePlugin; * @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(themeName, themesFolder, projectStaticAssetsOutputFolder) { + const themeFolder = path.resolve(themesFolder, themeName); + if (fs.existsSync(themeFolder)) { + logger.debug("Found theme ", themeName, " in folder ", themeFolder); - const themeFolder = path.resolve(themesFolder, themeName); - if(fs.existsSync(themeFolder)) { - logger.debug("Found theme ", themeName, " in folder ", themeFolder); + copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder); - copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder); + const themeFile = generateThemeFile(themeFolder, themeName); - const themeFile = generateThemeFile( - themeFolder, - themeName - ); - - fs.writeFileSync(path.resolve(themeFolder, themeName + '.js'), themeFile); - } + fs.writeFileSync(path.resolve(themeFolder, themeName + '.js'), themeFile); + return true; + } + return false; }; 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 1040de93be2..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 @@ -30,7 +30,6 @@ public void secondTheme_staticFilesNotCopied() { driver.getPageSource().contains("Could not navigate")); getDriver().get(getRootURL() + "/theme/no-copy/no-copy.txt"); - System.out.println(" === " + driver.getTitle()); Assert.assertTrue("no-copy theme should not be handled", driver.getPageSource().contains("Could not navigate to 'theme/no-copy/no-copy.txt'")); } From a5f958e1ba622a1a27d8c2f6d65f318d0caf1716 Mon Sep 17 00:00:00 2001 From: Mikael Grankvist Date: Tue, 17 Nov 2020 13:14:44 +0200 Subject: [PATCH 3/3] Update error text --- .../application-theme-plugin/application-theme-plugin.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 36862fe6129..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 @@ -81,7 +81,7 @@ class ApplicationThemePlugin { 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" + - "If extending the jar theme the 'parent' theme feature should be used"); + "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);