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.

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

Use unhashed browser urls, fixes #710 #964

Merged
merged 3 commits into from
May 9, 2021
Merged

Use unhashed browser urls, fixes #710 #964

merged 3 commits into from
May 9, 2021

Conversation

hubsif
Copy link
Contributor

@hubsif hubsif commented Mar 19, 2021

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.

@hubsif hubsif requested a review from a team as a code owner March 19, 2021 16:35
@relativeci
Copy link

relativeci bot commented Mar 19, 2021

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

8994b5b vs f09f821

Changed metrics (2/8)
Metric Current Baseline
Initial JS 1.61MB(~-0.01%) 1.61MB
Cache Invalidation 15.62% 0.17%
Changed assets by type (1/7)
            Current     Baseline
JS 8.12MB (~-0.01%) 8.12MB

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

@ghys
Copy link
Member

ghys commented Mar 20, 2021

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
Copy link
Contributor Author

hubsif commented Mar 22, 2021

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
Copy link
Member

ghys commented Mar 22, 2021

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
Copy link
Contributor Author

hubsif commented Mar 22, 2021

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
Copy link
Member

ghys commented Mar 22, 2021

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
Copy link
Contributor Author

hubsif commented Mar 23, 2021

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
Copy link
Contributor Author

hubsif commented Mar 23, 2021

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
Copy link
Member

ghys commented Mar 24, 2021

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
Copy link
Member

ghys commented Mar 24, 2021

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
Copy link
Member

ghys commented Mar 27, 2021

Found another one - the Blockly icon

image

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

@hubsif
Copy link
Contributor Author

hubsif commented Mar 27, 2021

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
Copy link
Contributor Author

hubsif commented Mar 29, 2021

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
Copy link
Member

ghys commented Apr 1, 2021

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

{
test: /\.(png|jpe?g|gif|svg)(\?.*)?$/,
loader: 'url-loader',
options: {
limit: 10000,
name: 'images/[name].[ext]'

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 hubsif requested a review from peuter as a code owner April 12, 2021 20:32
@hubsif
Copy link
Contributor Author

hubsif commented Apr 12, 2021

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
Copy link
Member

ghys commented Apr 18, 2021

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
Copy link
Contributor Author

hubsif commented Apr 27, 2021

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
Copy link
Member

ghys commented Apr 28, 2021

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.

Signed-off-by: Hubert Nusser <[email protected]>
@hubsif
Copy link
Contributor Author

hubsif commented Apr 28, 2021

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.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Seems all good now - everything seems to work pretty well. I believe it's ready for prime time at last. It might break a few bookmarks and the like for some people, but I'm not worried too much about this considering the benefits. Great change, many thanks!

@ghys ghys merged commit 71b0113 into openhab:main May 9, 2021
@hubsif hubsif deleted the fix_710 branch May 10, 2021 12:39
@ghys ghys added main ui Main UI enhancement New feature or request labels May 30, 2021
@ghys ghys added this to the 3.1 milestone May 30, 2021
@ghys ghys changed the title use unhashed browser urls, fixes #710 Use unhashed browser urls, fixes #710 May 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants