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: Serve theme static files from VAADIN/static #9451

Merged
merged 7 commits into from
Nov 24, 2020
Merged

feat: Serve theme static files from VAADIN/static #9451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b088d80
feat: Serve theme static files from VAADIN/static
caalador Nov 20, 2020
d25cc50
expect theme-loader
caalador Nov 20, 2020
84588d1
update: theme-loader only updates url if file exists by path.
caalador Nov 20, 2020
25d1292
Merge branch 'master' into issues/9405_theme_static_files
caalador Nov 20, 2020
cb7d2d5
Fix theme test paths
caalador Nov 20, 2020
7e63eb7
Handle CSS imports with theme-loader
caalador Nov 23, 2020
9563620
Accept ./ and ../ on URL
caalador Nov 23, 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
8 changes: 4 additions & 4 deletions flow-server/src/main/java/com/vaadin/flow/server/StaticFileServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@

import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;

import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
Expand Down Expand Up @@ -111,7 +110,7 @@ public boolean serveStaticResource(HttpServletRequest request,
}

URL resourceUrl = null;
if (isAllowedVAADINBuildUrl(filenameWithPath)) {
if (isAllowedVAADINBuildOrStaticUrl(filenameWithPath)) {
resourceUrl = servletService.getClassLoader()
.getResource("META-INF" + filenameWithPath);
}
Expand Down Expand Up @@ -195,9 +194,10 @@ private String fixIncorrectWebjarPath(String requestFilename) {
* requested filename containing path
* @return true if we are ok to try serving the file
*/
private boolean isAllowedVAADINBuildUrl(String filenameWithPath) {
private boolean isAllowedVAADINBuildOrStaticUrl(String filenameWithPath) {
// Check that we target VAADIN/build
return filenameWithPath.startsWith("/" + VAADIN_BUILD_FILES_PATH);
return filenameWithPath.startsWith("/" + VAADIN_BUILD_FILES_PATH)
|| filenameWithPath.startsWith("/" + VAADIN_STATIC_FILES_PATH);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion flow-server/src/main/java/com/vaadin/flow/server/frontend/TaskUpdateWebpack.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import static com.vaadin.flow.server.frontend.FrontendUtils.TARGET;
import static com.vaadin.flow.server.frontend.FrontendUtils.WEBPACK_CONFIG;
import static com.vaadin.flow.server.frontend.FrontendUtils.WEBPACK_GENERATED;
import static com.vaadin.flow.shared.ApplicationConstants.VAADIN_STATIC_FILES_PATH;

/**
* Updates the webpack config file according with current project settings.
Expand Down Expand Up @@ -90,7 +91,8 @@ public class TaskUpdateWebpack implements FallibleCommand {
this.webpackConfigPath = webpackConfigFolder.toPath();
this.useV14Bootstrapping = useV14Bootstrapping;
this.flowResourcesFolder = flowResourcesFolder.toPath();
this.resourceFolder = new File(webpackOutputDirectory.getParentFile(), "resources").toPath();
this.resourceFolder = new File(webpackOutputDirectory.getParentFile(),
VAADIN_STATIC_FILES_PATH).toPath();
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -110,7 +110,7 @@ function handleThemes(themeName, themesFolder, projectStaticAssetsOutputFolder)
if (fs.existsSync(themeFolder)) {
logger.debug("Found theme ", themeName, " in folder ", themeFolder);

copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder);
copyThemeResources(themeFolder, projectStaticAssetsOutputFolder);

const themeFile = generateThemeFile(themeFolder, themeName);

Expand Down
5 changes: 3 additions & 2 deletions flow-server/src/main/resources/plugins/application-theme-plugin/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
{
"description": "application-theme-plugin",
"keywords": [
"plugin"
"plugin",
"application theme"
],
"repository": "vaadin/flow",
"name": "@vaadin/application-theme-plugin",
"version": "0.1.1",
"version": "0.1.3",
"main": "application-theme-plugin.js",
"author": "Vaadin Ltd",
"license": "Apache-2.0",
Expand Down
15 changes: 4 additions & 11 deletions flow-server/src/main/resources/plugins/application-theme-plugin/theme-copy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,30 +16,23 @@

/**
* This file handles copying of theme files to
* [staticResourcesFolder]/theme/[theme-name]
* [staticResourcesFolder]
*/

const fs = require('fs');
const path = require('path');

/**
* create theme/themeName folders and copy theme files there.
* copy theme files to static assets folder.
pleku marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {String} themeName name of theme we are handling
* @param {string} themeFolder Folder with theme file
* @param {string} projectStaticAssetsOutputFolder resources output folder
*/
function copyThemeResources(themeName, themeFolder, projectStaticAssetsOutputFolder) {
function copyThemeResources(themeFolder, projectStaticAssetsOutputFolder) {
if (!fs.existsSync(path.resolve(projectStaticAssetsOutputFolder))) {
require('mkdirp')(path.resolve(projectStaticAssetsOutputFolder));
}
if (!fs.existsSync(path.resolve(projectStaticAssetsOutputFolder, "theme"))) {
fs.mkdirSync(path.resolve(projectStaticAssetsOutputFolder, "theme"));
}
if (!fs.existsSync(path.resolve(projectStaticAssetsOutputFolder, "theme", themeName))) {
fs.mkdirSync(path.resolve(projectStaticAssetsOutputFolder, "theme", themeName));
}
copyThemeFiles(themeFolder, path.resolve(projectStaticAssetsOutputFolder, "theme", themeName));
copyThemeFiles(themeFolder, projectStaticAssetsOutputFolder);
}

const ignoredFileExtensions = [".css", ".js", ".json"];
Expand Down
19 changes: 19 additions & 0 deletions flow-server/src/main/resources/plugins/theme-loader/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{
"description": "theme-loader updates css urls targeting './' to instead be 'VAADIN/static/' as used by app theme",
"keywords": [
"plugin",
"application theme"
],
"repository": "vaadin/flow",
"name": "@vaadin/theme-loader",
"version": "0.0.1",
"main": "theme-loader.js",
"author": "Vaadin Ltd",
"license": "Apache-2.0",
"bugs": {
"url": "https://github.com/vaadin/flow/issues"
},
"files": [
"theme-loader.js"
]
}
12 changes: 12 additions & 0 deletions flow-server/src/main/resources/plugins/theme-loader/theme-loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
module.exports = function (source, map) {
// Collect groups [url(] [ |'|"]optional './' and end of url
source = source.replace(/(url\()(\'|\")?(\.\/)(.*\2\))/g, function (match, url, quoteMark, replace, endString) {
// keep the url the same except replace the ./ to VAADIN/static
if (quoteMark) {
return url + quoteMark + 'VAADIN/static/' + endString;
}
return url + 'VAADIN/static/' + endString;
});

this.callback(null, source, map);
}
3 changes: 2 additions & 1 deletion flow-server/src/main/resources/plugins/webpack-plugins.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"plugins": [
"stats-plugin",
"application-theme-plugin"
"application-theme-plugin",
"theme-loader"
]
}
7 changes: 6 additions & 1 deletion flow-server/src/main/resources/webpack.generated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,13 +173,18 @@ module.exports = {
options: {
url: (url, resourcePath) => {
// do not handle theme folder url files
if(url.startsWith("theme")) {
if (url.startsWith("VAADIN/static")) {
return false;
}
return true;
},
},
},
{
// theme-loader will change any url starting with './' to start with 'VAADIN/static' instead
// NOTE! this loader should be here so it's run before css-loader as loaders are applied Right-To-Left
loader: '@vaadin/theme-loader'
}
],
},
]
Expand Down
31 changes: 31 additions & 0 deletions flow-server/src/test/java/com/vaadin/flow/server/StaticFileServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,37 @@ public void serveStaticBundleBuildResource() throws IOException {
assertBundleBuildResource(pathInfo);
}

@Test
public void contextAndServletPath_serveStaticFileResource()
throws IOException {
String pathInfo = "/VAADIN/static/img/bg.jpg";
setupRequestURI("/context", "/servlet", pathInfo);
assertBundleBuildResource(pathInfo);
}

@Test
public void ServletPath_serveStaticFileResource()
throws IOException {
String pathInfo = "/VAADIN/static/img/bg.jpg";
setupRequestURI("", "/servlet", pathInfo);
assertBundleBuildResource(pathInfo);
}

@Test
public void contextPath_serveStaticFileResource()
throws IOException {
String pathInfo = "/VAADIN/static/img/bg.jpg";
setupRequestURI("/context", "", pathInfo);
assertBundleBuildResource(pathInfo);
}

@Test
public void serveStaticFileResource() throws IOException {
String pathInfo = "/VAADIN/static/img/bg.jpg";
setupRequestURI("", "", pathInfo);
assertBundleBuildResource(pathInfo);
}

pleku marked this conversation as resolved.
Show resolved Hide resolved
public void assertBundleBuildResource(String pathInfo) throws IOException {
byte[] fileData = "function() {eval('foo');};"
.getBytes(StandardCharsets.UTF_8);
Expand Down
2 changes: 1 addition & 1 deletion flow-tests/test-themes/frontend/theme/app-theme/global.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
body {
background-image: url('theme/app-theme/img/bg.jpg');
background-image: url("./img/bg.jpg");
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that exactly the opposite of #9430? @pleku

Copy link
Contributor Author

@caalador caalador Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

hmm... but now it's a direct reference from the css file so IDEA (and others) can for instance auto complete and validate the reference file.

Copy link
Contributor

@knoobie knoobie Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's a convenient for developer for "small" projects but can be a huge burden for dev or ops teams in bigger projects when you have requirements to load images from something outside of the app.

At least this "magic" should be configurable(opt-out) if that's something you need to have for vaadin's DX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated so that we only change the url if the file actually exists relative to the css being handled.

Copy link
Contributor

@knoobie knoobie Nov 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting idea!

Just a test case to verify: Project setup: Vaadin + Spring Boot

resources/public/img/first.png
resources/META-INF/VAADIN/static/img/second.jpg
resources/VAADIN/themes/my-theme/global.css

global.css:

body {
 background-image: url("./img/first.png");
}
main {
 background-image: url("./img/second.jpg");
}

Spring boot would serve the image first.png because it is inside the public folder. Now your change would not change the background-image of the css because first.png isn't reachable for webpack. And the background-image of main would be rewritten and of course served successfully. Correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The theme-loader will now actually check that the file exists relative to the css file handled so: if it can find the file resources/VAADIN/themes/my-theme/img/second.jpg it will be rewritten with VAADIN/static else it's seen as "external" and supplied by someone else.

Copy link
Contributor

Choose a reason for hiding this comment

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

That was the missing piece, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we check that the file exists either in the folder or inside a the parent theme .jar 👀 and only modify it then, we would be handling only the URLs that we should. But then ofc. it might be quite hard for the users to detect typos (?) if those only result in 404 on the page. Could do something for that, e.g. make them more visible in development mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for dev mode we now log any url not modified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets add another test which verifies that `url("../icons/icon.png"); or similar works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we have a small problem in that we won't accept static serving for anything that contains ../
What we could do would be to accept navigation inside the theme folder and rewrite those to be absolute to the file target so we wouldn't have ../ e.g. ../icons/icon.png would fail from /app-theme/global.css, but would be rewritten to icons/icon.png in app-theme/sub/extras.css

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option could be to use a magic URL thingy there like theme:// but not sure if it should resolve to

  1. to the theme/ folder
  2. to the currently active theme
    Usually it should resolve to 2) but in some cases when you're creating a reusable theme it should refer to the parent theme and not the currently active theme ... so probably this is just unnecessary complication and thus a bad idea.

I think using e.g. ../icons/icon.png is common so if we can just automatically handle it, lets go with that.
For Java code, we'll expect people to anyway use the full path "frontend/theme/my-theme/icons/icon.png".

One thing to note, even though it is not directly part of this issue, that the npm-based imports it is supposed to work with:
"foobar/images/image.png" when the package has been declared to be imported in theme.json. These URLs should not be touched even though they are not "external".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the format "foobar/images/image.png" they won't be touched. Only ./is handled.

}
7 changes: 0 additions & 7 deletions flow-tests/test-themes/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,6 @@
<!-- 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>
Expand Down