hubsif commented 3 years ago

I think I did it 🕺 😄.

I wanted to make this clean, so it wouldn't require to maintain all pathes in the UIService. I did a lot of research and it turned out to be quite complicated with no experience at all in this osgi bundling and embedded jetty and how to register a rewrite handler with this.

In the end the change is so simple:

           URL url = defaultHttpContext.getResource(name);
           return (url != null) ? url : defaultHttpContext.getResource(APP_BASE + "/index.html");

getResource returns null if the resource can't be found (which assume to be those resulting in 404 then). If that's the case, return index.html instead. And no need to take care of any path prefixes, as pathes to other registered contexts don't get routed to this context.

relativeci[bot] commented 3 years ago

Job #98: Bundle Size — 10.41MB (~-0.01%).

8994b5b415fba4034e4d05854475e6e78fae4546 vs f09f821f8c781185764c5d5544ba3b93e9a1358b

Changed metrics (2/8)

| Metric | Current | Baseline | |---|--:|--:| | [Initial JS](https://app.relative-ci.com/projects/ZNG5hy4VeSJQVQcq1Kvu/jobs/98-5bdca802-e57c-4994-ad92-7f117fcd24cf/assets?ba=%7B%22filters%22%3A%7B%22et.entrypoint%22%3Afalse%2C%22et.initial%22%3Atrue%2C%22et.chunk%22%3Afalse%2C%22et.asset%22%3Afalse%2C%22ft.CSS%22%3Afalse%2C%22ft.JS%22%3Atrue%2C%22ft.IMG%22%3Afalse%2C%22ft.MEDIA%22%3Afalse%2C%22ft.FONT%22%3Afalse%2C%22ft.HTML%22%3Afalse%2C%22ft.OTHER%22%3Afalse%7D%7D "View all initial JS assets") | `1.61MB`(`~-0.01%`) | `1.61MB` | | [Cache Invalidation](https://app.relative-ci.com/projects/ZNG5hy4VeSJQVQcq1Kvu/jobs/98-5bdca802-e57c-4994-ad92-7f117fcd24cf/assets?ba=%7B%22filters%22%3A%7B%22changed%22%3Atrue%7D%7D "View all changed assets") | `15.62%` | `0.17%` |

Changed assets by type (1/7)

| | Current | Baseline | |---|--:|--:| | [JS](https://app.relative-ci.com/projects/ZNG5hy4VeSJQVQcq1Kvu/jobs/98-5bdca802-e57c-4994-ad92-7f117fcd24cf/assets?ba=%7B%22filters%22%3A%7B%22ft.CSS%22%3Afalse%2C%22ft.JS%22%3Atrue%2C%22ft.IMG%22%3Afalse%2C%22ft.MEDIA%22%3Afalse%2C%22ft.FONT%22%3Afalse%2C%22ft.HTML%22%3Afalse%2C%22ft.OTHER%22%3Afalse%7D%7D "View all JS assets") | `8.12MB` (`~-0.01%`) | `8.12MB` |

View Job #98 report on app.relative-ci.com

ghys commented 3 years ago

At first sight it seems to work well! Obviously redirecting every URL that don't match a resource to index.html could have side effects, but I didn't find any yet.

Small bug though: you have relative URLs in index.html and a few Vue components like res/... and those don't work if you don't start the navigation at the root level - the favicon doesn't appear, etc. There might be some other cases. Replacing them with server-relative URLs i.e. /res/... should be enough, though it will be even more difficult to allow users to run openHAB in a custom path like https://example.com/openhab/ behind a reverse proxy if they wish (doesn't work now either, see #432).

hubsif commented 3 years ago

Obviously redirecting every URL that don't match a resource to index.html could have side effects, but I didn't find any yet.

Well, that's the same as the webpack devserver does (except for files containing dots, by default) 🤷. I can't think of any side effects when loading the app instead of a 404.

Small bug though: you have relative URLs in index.html and a few Vue components like res/...

oh, good catch! Actually, I only checked for errorneous requests, not those with invalid content 😳.

The conflict

though it will be even more difficult to allow users to run openHAB in a custom path like https://example.com/openhab/ behind a reverse proxy if they wish (doesn't work now either, see #432).

I've had a closer look at all this and did some research. The result being, in short, that working links and relative paths seem to be mutually exclusive 😞.

Longer explanation: For relocation, relative paths are required (which is currently the case). For a Framework7 SPA using its own router and the history API to make links work, absolute paths are required (see also similarity to the Vue-cli docs). Since the main UI is a "prebuilt" JS application shipped with OH3, this can't be changed dynamically. The app's "publicPath", set in webpack config, is compiled statically into the app build.

Right now it should be generally possible to put the mainUI under an arbitrary subpath (special issues like apache auth put aside - and I don't know about the core services). With this change, this would not be possible anymore, the app could only be served under root (/) and forwarded as such.

I found some addons that would allow for a "dynamic" publicpath, like webpack-require-from. This could be used to try to determine a subpath before loading or even load it as config parameter from core. Though, this would require extra work, of course.

The only thing I don't fully understand about all this yet, is why the links generated by f7 cannot simply include the hashbang...

So, @ghys, unless you know more than I do, I think a decision is required here: Keep up relative pathing, making it possible to have OH3 mapped to arbitrary locations, or making links work ...

ghys commented 3 years ago

Right now it should be generally possible to put the mainUI under an arbitrary subpath (special issues like apache auth put aside - and I don't know about the core services). With this change, this would not be possible anymore, the app could only be served under root (/) and forwarded as such.

For these cases I believe this could be tackled with rewrite rules in the reverse proxy, at least for now (if a dedicated subdomain cannot be used). That is, intercept /rest/* and forward them to /openhab/rest/*, and so on, this is something that NGINX and friends can do - not only in URLs but also in the contents themselves.

The only thing I don't fully understand about all this yet, is why the links generated by f7 cannot simply include the hashbang...

Me neither 🤷

So, @ghys, unless you know more than I do, I think a decision is required here: Keep up relative pathing, making it possible to have OH3 mapped to arbitrary locations, or making links work ...

I like the clean URLs without #!/ :) so yeah if we can solve all of them gotchas it will be nice if it works! Responding to non-existent resources with index.html is a little weird, but ultimately it's fine IMO.

oh, good catch! Actually, I only checked for errorneous requests, not those with invalid content 😳.

Found 2 other ones! In the profile page, there are links to changePassword and createApiToken which should now be resp. /changePassword & /createApiToken.

hubsif commented 3 years ago

For these cases I believe this could be tackled with rewrite rules in the reverse proxy, at least for now (if a dedicated subdomain cannot be used). That is, intercept /rest/ and forward them to /openhab/rest/, and so on, this is something that NGINX and friends can do - not only in URLs but also in the contents themselves.

Actually, it's the other way round: the reverse proxy would intercept /openhab/rest/ and forward it to /rest/ at openHAB 😉 And yes, such content rewrite filters exist, but they seem to be the last resort, probably because they cause too many problems ...

Anyway, I'm currently following up my first idea (rewritehandler) which might eventually solve all this. If that turns out to be an impasse, I will finish this PR off.

ghys commented 3 years ago

Actually, it's the other way round: the reverse proxy would intercept /openhab/rest/ and forward it to /rest/ at openHAB 😉 And yes, such content rewrite filters exist, but they seem to be the last resort, probably because they cause too many problems ...

No what I meant is, if you "proxy_pass" openHAB in a subpath e.g. /openhab and the UI incidentally tries to request /rest/something (outside the subpath), interpret (rewrite) it as /openhab/rest/something so consider it part of openHAB (software defining something under /rest/ being rather unusual in my experience).

hubsif commented 3 years ago

No what I meant is, if you "proxy_pass" openHAB in a subpath e.g. /openhab and the UI incidentally tries to request /rest/something (outside the subpath), interpret (rewrite) it as /openhab/rest/something so consider it part of openHAB (software defining something under /rest/ being rather unusual in my experience).

Hm, I'm still somehow not able to fit this to my statement you referred to 🤔, which was:

With this change, this would not be possible anymore, the app could only be served under root (/) and forwarded as such.

Anyway, I couldn't find a clean solution around this, so I fixed the missing links. The PR is now complete, meaning the hashbang is gone, which makes it possible to open links in new tabs, and relocating the ui is only possible by more complex means like content rewriting.

hubsif commented 3 years ago

Ah, what I forgot to ask, @ghys, with regards to #432: What can still be done with all this, is to (permanently) locate the main UI under some subpath, like "/ui". Any intention of doing so? Edit: see also my latest comment in #432 as to what difference it makes for reverse proxying having mainUI run in a subpath.

ghys commented 3 years ago

What can still be done with all this, is to (permanently) locate the main UI under some subpath, like "/ui". Any intention of doing so?

Nah. I like the main UI being served from the root path - I feel it's "cleaner" and tried to make it work that way (it was not as straightforward as you think!). Of course I could cope with OH2 UIs served from their own subpaths but if you want to have an all-encompassing UI I felt it shouldn't be "relegated" to a subpath. Maybe @kaikreuzer could offer his take on this.

ghys commented 3 years ago

Btw @hubsif - this PR is now my main system's UI so if it ends up working well you can expect a merge soon.

ghys commented 3 years ago

Found another one - the Blockly icon

I think a search and replace of "res/ could do wonders.

hubsif commented 3 years ago

I think a search and replace of "res/ could do wonders.

Argh. Actually, I did a lot of regex searches in vscode, don't know how I missed res/... Just found more of them, will correct.

hubsif commented 3 years ago

ok, so I found some more res/ occurrences. Though it seems I still have to look deeper into some, as I don't understand how they work yet, e.g. the logo in app.vue, which seems to get cached and/or embedded by webpack somehow? Actually, I thought res is a path on the openhab server, but it's also a subpath of the src folder ...

And then there are the sidebar tiles. For me the UIs returned by the server (/rest/ui/tiles) currently contains:

[
  {
    "name": "Basic UI",
    "url": "../basicui/app",
    "imageUrl": "res/img/basicui.png"
  },
  {
    "name": "HABPanel",
    "url": "../habpanel/index.html",
    "imageUrl": "../habpanel/tile.png"
  }
]

This is somewhat inconsistent but works just fine on the home page ("root path"). Is the right sidepanel actually shown anywhere else?

ghys commented 3 years ago

ok, so I found some more res/ occurrences. Though it seems I still have to look deeper into some, as I don't understand how they work yet, e.g. the logo in app.vue, which seems to get cached and/or embedded by webpack somehow?

Yes webpack's url-loader will convert images to base64 if they're small enough (currently 10kB, never changed it from the default config set by the f7 app creator). https://webpack.js.org/loaders/url-loader/#limit https://github.com/openhab/openhab-webui/blob/545174f36c8e2eae36d4ba210f930f6ec57091ee/bundles/org.openhab.ui/web/build/webpack.config.js#L152-L157

This is somewhat inconsistent but works just fine on the home page ("root path"). Is the right sidepanel actually shown anywhere else?

If you begin navigation from a subpath say /settings and then navigate back to the home page, some of the relative URLs (for instance Basic UI) will not work indeed. other-apps.vue has some nasty hacks related to this (replacements) because I initially didn't want to modify the "real" tile URLs since I wanted to maintain compatibility with the OH2 dashboard. Now I guess this can be fixed directly at the source in the Java classes for the tiles (using server-relative URLs would probably be best).

hubsif commented 3 years ago

So, I did some research regarding webpack and that "url-loader" and found out that this plugin actually processes all images referenced in the code and either integrates them inline as base64 or copies them to the target path /images (set in webpack.config.js) and automatically adjusts the img-tag src references in the code accordingly (if the src path is relative or starts with @). I wondered what this might actually be good for and eventually found the answer in the webpack assets docs: It allows you to store images alongside the code they belong to, meaning you can have everything that belongs to a component under its component folder. I really like that idea, so I moved the chartdesigner svgs to their chart component.

What actually happens with current settings is that images (>10KB) get "url-loaded" to the /images path and the whole /res folder gets copied (by the "CopyWebpackPlugin") to the destination folder /res. This leads to the same images being included twice in the build (ok, they would be included twice if any of them was >10KB)

I learned that the /res folder is actually meant to include "static" content only that doesn't get sourced by the code and thus not processed by any loader, and that other images should not be put there. I then started cleaning up a lot and had to stop myself halfway to not do a lot of work that in the end might not be desired (e.g. IMHO the assets folder is meant to carry all "assets" like CSS, fonts, images, etc. which is currently not the case. And the "res" folder is outdated and its better naming would be "static", lots of cordova stuff that probably won't ever be used, etc.).

So, with only doing what's actually required the result now is as follows:

I moved the charts images to their component folder
I kept the "static" images and icons as is and adjusted their references to be absolute
I created a folder /images (according to /fonts and /css) that holds the "url-loaded" global images that don't belong to a certain component (like "blockly.svg")
I adjusted the basicui and cometvisu tiles path to be absolute
ah, and I adjusted the "apple-touch-icon" to have a white background as it didn't look too nice (iOS created a black square around the white circle, I think someone in the forum also complained)

This should eventually fix all issues with image sourcing. Though tests with a real prod-build are still outstanding.

ghys commented 3 years ago

And the "res" folder is outdated and its better naming would be "static", lots of cordova stuff that probably won't ever be used, etc.

It was originally named static actually and it was not because of Cordova, but because it could be confused, or even conflicting with the /static assets served from the $OPENHAB_CONF/html folder, so it was renamed to res.

hubsif commented 3 years ago

ok, so I finalized this with some more small changes:

changed the urls and imageUrls of BasicUI, HABPanel and HABot to be absolute
removed the "../"-replacement from panel-right
also removed it from the file other-apps.vue you mentioned, though that file seems unused (shall I remove it and its broken sourcing in overview-tab?)

Though tests with a real prod-build are still outstanding.

I just discovered that I cannot build a complete UI package, e.g. BasicUI fails with some (supposedly gulp version related) error. But I created a build for mainUI and HABPanel and it seems just fine. So, I'd say that test is passed.

ghys commented 3 years ago

also removed it from the file other-apps.vue you mentioned, though that file seems unused (shall I remove it and its broken sourcing in overview-tab?)

Indeed this was an early version when layout pages didn't exist and the overview tab was basically the other apps tiles (similar to OH2's dashboard). I'd say it can be removed.

hubsif commented 3 years ago

I'd say it can be removed.

Ok. So, I removed everything in "overview-tab" that seemed unused (including expandable-card.vue)

Additionally, I rebased this PR for the Jenkins build to succeed.

openhab / openhab-webui

Use unhashed browser urls, fixes #710 #964

Job #98: Bundle Size — 10.41MB (~-0.01%).

The conflict