Skip to content

Commit

Permalink
fix: theme files can now be referenced as theme/theme-name (#9590)
Browse files Browse the repository at this point in the history 
Theme files are now copied under theme/[theme-name]
and can be referenced by theme/them-name/path/file.ff
even though they are located at VAADIN/static

Fixes: #9405 and #9535
  • Loading branch information
@caalador
caalador committed Dec 10, 2020
1 parent 93a0be2 commit 5703853
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 37 deletions.
13 changes: 9 additions & 4 deletions flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import javax.servlet.ServletOutputStream;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
Expand Down Expand Up @@ -59,6 +58,7 @@
import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_OPTIONS;
import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_SUCCESS_PATTERN;
import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_TIMEOUT;
import static com.vaadin.flow.server.StaticFileServer.APP_THEME_PATTERN;
import static com.vaadin.flow.server.frontend.FrontendUtils.GREEN;
import static com.vaadin.flow.server.frontend.FrontendUtils.RED;
import static com.vaadin.flow.server.frontend.FrontendUtils.commandToString;
Expand Down Expand Up @@ -285,9 +285,9 @@ private static DevModeHandler createInstance(int runningPort, Lookup lookup,
*/
public boolean isDevModeRequest(HttpServletRequest request) {
String pathInfo = request.getPathInfo();
return pathInfo != null && pathInfo.startsWith("/" + VAADIN_MAPPING)
&& !pathInfo
.startsWith("/" + StreamRequestHandler.DYN_RES_PREFIX);
return pathInfo != null && (pathInfo.startsWith("/" + VAADIN_MAPPING)
|| APP_THEME_PATTERN.matcher(pathInfo).find()) && !pathInfo
.startsWith("/" + StreamRequestHandler.DYN_RES_PREFIX);
}

/**
Expand Down Expand Up @@ -325,6 +325,11 @@ public boolean serveDevModeRequest(HttpServletRequest request,
return true;
}

// Redirect theme source request
if(APP_THEME_PATTERN.matcher(requestFilename).find()) {
requestFilename = "/VAADIN/static" + requestFilename;
}

HttpURLConnection connection = prepareConnection(requestFilename,
request.getMethod());

Expand Down
24 changes: 18 additions & 6 deletions flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ public class StaticFileServer implements StaticFileHandler {
private final VaadinServletService servletService;
private DeploymentConfiguration deploymentConfiguration;

// Matcher to match string starting with '/theme/[theme-name]/'
protected static final Pattern APP_THEME_PATTERN = Pattern
.compile("^\\/theme\\/[\\s\\S]+?\\/");

/**
* Constructs a file server.
*
Expand All @@ -80,8 +84,9 @@ public boolean isStaticResourceRequest(HttpServletRequest request) {
return false;
}

if (requestFilename.startsWith("/" + VAADIN_STATIC_FILES_PATH)
|| requestFilename.startsWith("/" + VAADIN_BUILD_FILES_PATH)) {
if (APP_THEME_PATTERN.matcher(requestFilename).find() || requestFilename
.startsWith("/" + VAADIN_STATIC_FILES_PATH) || requestFilename
.startsWith("/" + VAADIN_BUILD_FILES_PATH)) {
// The path is reserved for internal resources only
// We rather serve 404 than let it fall through
return true;
Expand Down Expand Up @@ -111,8 +116,13 @@ public boolean serveStaticResource(HttpServletRequest request,

URL resourceUrl = null;
if (isAllowedVAADINBuildOrStaticUrl(filenameWithPath)) {
resourceUrl = servletService.getClassLoader()
if(APP_THEME_PATTERN.matcher(filenameWithPath).find()) {
resourceUrl = servletService.getClassLoader()
.getResource("META-INF/VAADIN/static" + filenameWithPath);
} else {
resourceUrl = servletService.getClassLoader()
.getResource("META-INF" + filenameWithPath);
}
}
if (resourceUrl == null) {
resourceUrl = servletService.getStaticResource(filenameWithPath);
Expand Down Expand Up @@ -195,9 +205,10 @@ private String fixIncorrectWebjarPath(String requestFilename) {
* @return true if we are ok to try serving the file
*/
private boolean isAllowedVAADINBuildOrStaticUrl(String filenameWithPath) {
// Check that we target VAADIN/build
// Check that we target VAADIN/build | VAADIN/static | theme/theme-name
return filenameWithPath.startsWith("/" + VAADIN_BUILD_FILES_PATH)
|| filenameWithPath.startsWith("/" + VAADIN_STATIC_FILES_PATH);
|| filenameWithPath.startsWith("/" + VAADIN_STATIC_FILES_PATH)
|| APP_THEME_PATTERN.matcher(filenameWithPath).find();
}

/**
Expand Down Expand Up @@ -288,7 +299,8 @@ String getRequestFilename(HttpServletRequest request) {
// /VAADIN/folder/file.js
if (request.getPathInfo() == null) {
return request.getServletPath();
} else if (request.getPathInfo().startsWith("/" + VAADIN_MAPPING)) {
} else if (request.getPathInfo().startsWith("/" + VAADIN_MAPPING)
|| APP_THEME_PATTERN.matcher(request.getPathInfo()).find()) {
return request.getPathInfo();
}
return request.getServletPath() + request.getPathInfo();
Expand Down
2 changes: 1 addition & 1 deletion flow-server/src/main/resources/plugins/application-theme-plugin/application-theme-plugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ function handleThemes(themeName, themesFolder, projectStaticAssetsOutputFolder)

const themeProperties = getThemeProperties(themeFolder);

copyStaticAssets(themeProperties, projectStaticAssetsOutputFolder, logger);
copyStaticAssets(themeName, themeProperties, projectStaticAssetsOutputFolder, logger);

const themeFile = generateThemeFile(themeFolder, themeName, themeProperties);

Expand Down
2 changes: 1 addition & 1 deletion flow-server/src/main/resources/plugins/application-theme-plugin/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
],
"repository": "vaadin/flow",
"name": "@vaadin/application-theme-plugin",
"version": "0.2.2",
"version": "0.2.3",
"main": "application-theme-plugin.js",
"author": "Vaadin Ltd",
"license": "Apache-2.0",
Expand Down
19 changes: 12 additions & 7 deletions flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,12 @@ const glob = require('glob');
*
* Note! there can be multiple copy-rules with target folders for one npm package asset.
*
* @param {json} themeProperties
* @param {string} projectStaticAssetsOutputFolder
* @param {string} themeName name of the theme we are copying assets for
* @param {json} themeProperties theme properties json with data on assets
* @param {string} projectStaticAssetsOutputFolder project output folder where we copy assets to under theme/[themeName]
* @param {logger} theme plugin logger
*/
function copyStaticAssets(themeProperties, projectStaticAssetsOutputFolder, logger) {
function copyStaticAssets(themeName, themeProperties, projectStaticAssetsOutputFolder, logger) {

const assets = themeProperties['assets'];
if (!assets) {
Expand All @@ -65,15 +66,19 @@ function copyStaticAssets(themeProperties, projectStaticAssetsOutputFolder, logg
const copyRules = assets[module];
Object.keys(copyRules).forEach((copyRule) => {
const nodeSources = path.resolve('node_modules/', module, copyRule);
const files = glob.sync(nodeSources, { nodir: true });
const targetFolder = path.resolve(projectStaticAssetsOutputFolder, copyRules[copyRule]);
const files = glob.sync(nodeSources, {nodir: true});
const targetFolder = path.resolve(projectStaticAssetsOutputFolder, "theme", themeName, copyRules[copyRule]);

fs.mkdirSync(targetFolder, {
recursive: true
});
files.forEach((file) => {
logger.trace("Copying: ", file, '=>', targetFolder);
fs.copyFileSync(file, path.resolve(targetFolder, path.basename(file)));
const copyTarget = path.resolve(targetFolder, path.basename(file));
// Only copy if target file doesn't exist or if file to copy is newer
if (!fs.existsSync(copyTarget) || fs.statSync(copyTarget).mtime < fs.statSync(file).mtime) {
logger.trace("Copying: ", file, '=>', targetFolder);
fs.copyFileSync(file, copyTarget);
}
});
});
});
Expand Down
8 changes: 2 additions & 6 deletions flow-server/src/main/resources/webpack.generated.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,12 +208,8 @@ module.exports = {
options: {
outputPath: 'static/',
name(resourcePath, resourceQuery) {
const urlResource = resourcePath.substring(frontendFolder.length);
if(urlResource.match(themePartRegex)){
return /^(\\|\/)theme\1[\s\S]*?\1(.*)/.exec(urlResource)[2].replace(/\\/, "/");
}
if(urlResource.match(/(\\|\/)node_modules\1/)) {
return /(\\|\/)node_modules\1(?!.*node_modules)([\S]*)/.exec(urlResource)[2].replace(/\\/g, "/");
if (resourcePath.match(/(\\|\/)node_modules\1/)) {
return /(\\|\/)node_modules\1(?!.*node_modules)([\S]+)/.exec(resourcePath)[2].replace(/\\/g, "/");
}
return '[path][name].[ext]';
}
Expand Down
6 changes: 3 additions & 3 deletions ...ication-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,20 +45,20 @@ public void applicationTheme_GlobalCss_isUsedOnlyInEmbeddeComponent() {
final TestBenchElement themedComponent = $("themed-component").first();

Assert.assertEquals(
"url(\"" + getRootURL() + "/VAADIN/static/img/bg.jpg\")",
"url(\"" + getRootURL() + "/VAADIN/static/theme/embedded-theme/img/bg.jpg\")",
themedComponent.getCssValue("background-image"));

Assert.assertEquals("Ostrich",
themedComponent.getCssValue("font-family"));

final WebElement body = findElement(By.tagName("body"));
Assert.assertNotEquals(
"url(\"" + getRootURL() + "/path/VAADIN/static/img/bg.jpg\")",
"url(\"" + getRootURL() + "/path/VAADIN/static/theme/embedded-theme/img/bg.jpg\")",
body.getCssValue("background-image"));

Assert.assertNotEquals("Ostrich", body.getCssValue("font-family"));

getDriver().get(getRootURL() + "/VAADIN/static/img/bg.jpg");
getDriver().get(getRootURL() + "/VAADIN/static/theme/embedded-theme/img/bg.jpg");
Assert.assertFalse("app-theme background file should be served",
driver.getPageSource().contains("Could not navigate"));
}
Expand Down
2 changes: 1 addition & 1 deletion flow-tests/test-themes/src/main/java/com/vaadin/flow/uitest/ui/theme/ThemeView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public ThemeView() {
faText.setId(FONTAWESOME_ID);

Image snowFlake = new Image(
"VAADIN/static/fortawesome/icons/snowflake.svg", "snowflake");
"theme/app-theme/fortawesome/icons/snowflake.svg", "snowflake");
snowFlake.setHeight("1em");
snowFlake.setId(SNOWFLAKE_ID);

Expand Down
19 changes: 11 additions & 8 deletions flow-tests/test-themes/src/test/java/com/vaadin/flow/uitest/ui/theme/ThemeIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,11 @@ public void typeScriptCssImport_stylesAreApplied() {

@Test
public void secondTheme_staticFilesNotCopied() {
getDriver().get(getRootURL() + "/path/VAADIN/static/img/bg.jpg");
getDriver().get(getRootURL() + "/path/theme/app-theme/img/bg.jpg");
Assert.assertFalse("app-theme static files should be copied",
driver.getPageSource().contains("HTTP ERROR 404 Not Found"));

getDriver().get(getRootURL() + "/path/VAADIN/static/no-copy.txt");
getDriver().get(getRootURL() + "/path/theme/app-theme/no-copy.txt");
Assert.assertTrue("no-copy theme should not be handled",
driver.getPageSource().contains("HTTP ERROR 404 Not Found"));
}
Expand All @@ -70,13 +70,15 @@ public void applicationTheme_GlobalCss_isUsed() {
checkLogsForErrors();

final WebElement body = findElement(By.tagName("body"));
// Note theme/app-theme gets VAADIN/static from the file-loader
Assert.assertEquals(
"url(\"" + getRootURL() + "/path/VAADIN/static/img/bg.jpg\")",
"url(\"" + getRootURL() + "/path/VAADIN/static/theme/app-theme/img/bg.jpg\")",
body.getCssValue("background-image"));

Assert.assertEquals("Ostrich", body.getCssValue("font-family"));

getDriver().get(getRootURL() + "/path/VAADIN/static/img/bg.jpg");
// Note theme/app-theme gets VAADIN/static from the file-loader
getDriver().get(getRootURL() + "/path/VAADIN/static/theme/app-theme/img/bg.jpg");
Assert.assertFalse("app-theme background file should be served",
driver.getPageSource().contains("Could not navigate"));
}
Expand Down Expand Up @@ -128,22 +130,23 @@ public void subCssWithRelativePath_urlPathIsNotRelative() {
open();
checkLogsForErrors();

// Note theme/app-theme gets VAADIN/static from the file-loader
Assert.assertEquals("Imported css file URLs should have been handled.",
"url(\"" + getRootURL()
+ "/path/VAADIN/static/icons/archive.png\")",
+ "/path/VAADIN/static/theme/app-theme/icons/archive.png\")",
$(SpanElement.class).id(SUB_COMPONENT_ID)
.getCssValue("background-image"));
}

@Test
public void staticModuleAsset_servedFromStatic() {
public void staticModuleAsset_servedFromAppTheme() {
open();
checkLogsForErrors();

Assert.assertEquals(
"Node assets should have been copied to 'VAADIN/static'",
"Node assets should have been copied to 'theme/app-theme'",
getRootURL()
+ "/path/VAADIN/static/fortawesome/icons/snowflake.svg",
+ "/path/theme/app-theme/fortawesome/icons/snowflake.svg",
$(ImageElement.class).id(SNOWFLAKE_ID).getAttribute("src"));

open(getRootURL() + "/path/" + $(ImageElement.class).id(SNOWFLAKE_ID)
Expand Down

0 comments on commit 5703853

Please sign in to comment.