From 2c9ca7e3858380778a44206d31090c1bd40fa8dd Mon Sep 17 00:00:00 2001 From: caalador Date: Thu, 10 Dec 2020 09:49:49 +0200 Subject: [PATCH] fix: theme files can now be referenced as theme/theme-name (#9590) 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 # Conflicts: # flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java # flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java --- .../vaadin/flow/server/DevModeHandler.java | 14 ++++++++--- .../vaadin/flow/server/StaticFileServer.java | 25 +++++++++++++------ .../application-theme-plugin.js | 2 +- .../application-theme-plugin/package.json | 2 +- .../application-theme-plugin/theme-copy.js | 19 ++++++++------ .../src/main/resources/webpack.generated.js | 8 ++---- .../flow/server/DevModeHandlerTest.java | 2 +- .../ApplicationThemeComponentIT.java | 6 ++--- .../flow/uitest/ui/theme/ThemeView.java | 2 +- .../vaadin/flow/uitest/ui/theme/ThemeIT.java | 19 ++++++++------ 10 files changed, 61 insertions(+), 38 deletions(-) diff --git a/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java b/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java index cb2f80e5133..e86dd7b461a 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java @@ -55,10 +55,12 @@ import com.vaadin.flow.server.frontend.FrontendTools; import com.vaadin.flow.server.frontend.FrontendUtils; +import static com.vaadin.flow.server.Constants.VAADIN_MAPPING; import static com.vaadin.flow.server.InitParameters.SERVLET_PARAMETER_DEVMODE_WEBPACK_ERROR_PATTERN; 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.YELLOW; @@ -280,9 +282,10 @@ private static DevModeHandler createInstance(int runningPort, * @return true if the request should be forwarded to webpack */ public boolean isDevModeRequest(HttpServletRequest request) { - final String pathInfo = request.getPathInfo(); - return pathInfo != null && pathInfo.matches(".+\\.js") && !pathInfo - .startsWith("/" + StreamRequestHandler.DYN_RES_PREFIX); + String pathInfo = request.getPathInfo(); + return pathInfo != null && (pathInfo.startsWith("/" + VAADIN_MAPPING) + || APP_THEME_PATTERN.matcher(pathInfo).find()) && !pathInfo + .startsWith("/" + StreamRequestHandler.DYN_RES_PREFIX); } /** @@ -320,6 +323,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()); diff --git a/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java b/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java index f01f56fa6f4..50a6a1697b2 100644 --- a/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java +++ b/flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java @@ -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. * @@ -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; @@ -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); @@ -203,10 +213,10 @@ private boolean isAllowedVAADINBuildOrStaticUrl(String filenameWithPath) { filenameWithPath); return false; } - - // 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(); } /** @@ -297,7 +307,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(); 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 2835fe78759..71aff4c0b5f 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 @@ -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); 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 d43bfa2382a..767607fdce9 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 @@ -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", diff --git a/flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js b/flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js index 34abd768b2d..377cb184a4f 100644 --- a/flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js +++ b/flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js @@ -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) { @@ -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); + } }); }); }); diff --git a/flow-server/src/main/resources/webpack.generated.js b/flow-server/src/main/resources/webpack.generated.js index ed1af2fe4f7..22c66a393e4 100644 --- a/flow-server/src/main/resources/webpack.generated.js +++ b/flow-server/src/main/resources/webpack.generated.js @@ -190,12 +190,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]'; } diff --git a/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java b/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java index 8e2484fbda7..8a9fb20da55 100644 --- a/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java +++ b/flow-server/src/test/java/com/vaadin/flow/server/DevModeHandlerTest.java @@ -356,7 +356,7 @@ public void webpack_forDifferentRequests_shouldHaveCorrectResponse() @Test public void vaadinServlet_forDifferentRequests_shouldHaveCorrectResponse() throws Exception { - HttpServletRequest request = prepareRequest("/foo.js"); + HttpServletRequest request = prepareRequest("/VAADIN/foo.js"); HttpServletResponse response = prepareResponse(); int port = prepareHttpServer(0, HTTP_OK, ""); diff --git a/flow-tests/test-embedding/test-embedding-application-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java b/flow-tests/test-embedding/test-embedding-application-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java index 7fc99edaaf0..84ffaec3d41 100644 --- a/flow-tests/test-embedding/test-embedding-application-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java +++ b/flow-tests/test-embedding/test-embedding-application-theme/src/test/java/com/vaadin/flow/webcomponent/ApplicationThemeComponentIT.java @@ -45,7 +45,7 @@ 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", @@ -53,12 +53,12 @@ public void applicationTheme_GlobalCss_isUsedOnlyInEmbeddeComponent() { 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")); } diff --git a/flow-tests/test-themes/src/main/java/com/vaadin/flow/uitest/ui/theme/ThemeView.java b/flow-tests/test-themes/src/main/java/com/vaadin/flow/uitest/ui/theme/ThemeView.java index 2829caf6959..22904b8768e 100644 --- a/flow-tests/test-themes/src/main/java/com/vaadin/flow/uitest/ui/theme/ThemeView.java +++ b/flow-tests/test-themes/src/main/java/com/vaadin/flow/uitest/ui/theme/ThemeView.java @@ -56,7 +56,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); 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 36c7ee162e5..ce5162e023c 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 @@ -38,11 +38,11 @@ public class ThemeIT extends ChromeBrowserTest { @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")); - 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")); } @@ -54,13 +54,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")); } @@ -112,22 +114,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)