Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Only handle annotated theme #9406

Merged
merged 4 commits into from
Nov 17, 2020
Merged

feat: Only handle annotated theme #9406

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c4514f8
feat: Only handle annotated theme
caalador Nov 16, 2020
a5a3f07
Merge branch 'master' into issues/9383_only_handle_target_theme
caalador Nov 16, 2020
3830fd5
Stricter handling on theme duplicates
caalador Nov 17, 2020
a5f958e
Update error text
caalador Nov 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 39 additions & 22 deletions flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand All @@ -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");
Expand All @@ -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<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;
}
}
}

pleku marked this conversation as resolved.
Show resolved Hide resolved
this.options.themeProjectFolders.forEach((themeProjectFolder) => {
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");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"If extending the jar theme the 'parent' theme feature should be used")

I'm not sure if this is enough for anyone who doesn't know about the it already. Maybe
"Extending another theme is possible by adding { "parent": "my-parent-theme" } entry to the theme.json file inside your theme's folder." would be explicit

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
});
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.");
}
});
}
Expand All @@ -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;
};
27 changes: 0 additions & 27 deletions flow-tests/test-misc/src/test/java/com/vaadin/flow/misc/ui/NpmThemedComponentIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,33 +26,6 @@

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();
// No exception for bg-image should exist
checkLogsForErrors();

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
public void importedClientSideComponentIsThemed() {
open();
Expand Down
18 changes: 15 additions & 3 deletions flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand Down