Skip to content

Remove leading slash in generated asset paths - #932

Closed
madeline-os wants to merge 2 commits into
developfrom
mh/fix-mobile-static-paths
Closed

madeline-os wants to merge 2 commits into
developfrom
mh/fix-mobile-static-paths

Conversation

@madeline-os

@madeline-os madeline-os commented May 27, 2022

Copy link
Copy Markdown
Collaborator

#831 changed the
way that we generate routes for static assets. The old approach would
place the assets at a path starting with "static". The new approach
added a leading slash, which, when rendered in URLs, would lead to
double slashes at the beginning of routes in places where the user had
not expected there to be a slash. This can lead to 404's for assets for
older projects.

This change brings the current behavior in line with the old behavior.

I have:

  • Based work on latest develop branch
  • Followed the contribution guide
  • Looked for lint in my changes with hlint . (lint found code you did not write can be left alone)
  • Run the test suite: $(nix-build -A selftest --no-out-link)
  • Updated the changelog
  • (Optional) Run CI tests locally: nix-build release.nix -A build.x86_64-linux --no-out-link (or x86_64-darwin on macOS)

[#831](#831) changed the
way that we generate routes for static assets. The old approach would
place the assets at a path starting with "static". The new approach
added a leading slash, which, when rendered in URLs, would lead to
double slashes at the beginning of routes.

This change brings the new behavior in line with the old behavior.
@madeline-os

Copy link
Copy Markdown
Collaborator Author

This would be undoing a breaking change that was made a long time ago, but that itself is a breaking change. So I am leaving this up for consideration.

@madeline-os
madeline-os marked this pull request as draft May 27, 2022 17:53
@alexfmpe
alexfmpe requested a review from ali-abrar May 27, 2022 17:53
@ali-abrar

Copy link
Copy Markdown
Member

Obelisk also includes a <base href="/"> tag in the frontend html when building for the web or android, but not for ios. The value of the base tag is not currently configurable.

When that base tag is present, relative URLs (e.g., static/x as opposed to /static/x) should exhibit the same behavior as absolute URLs. If that base tag is absent and the route is set to something other than "/" (because the user's navigated to some subpath), the relative URL reference won't work.

Ultimately, we will want the base url to be configurable (likely via the route config). If it's set properly, the relative and absolute URLs should, again, resolve to the same thing.

Are there known cases where the change from !831 results in broken asset links? If not, perhaps the addition/removal of the prefix slash is not a noticeable change, even if it is technically a breaking one. It seems like we're redundantly absolute right now, and there's little practical difference from the perspective of users.

The exception for iOS and the behavior on Android both bear further investigation and documentation before any change is made.

@alexfmpe

Copy link
Copy Markdown
Contributor

Are there known cases where the change from !831 results in broken asset links?

Absolutely, that's how we noticed this - some images broke after bumping obelisk.
The ones that used to be <route>/static/foo turned to <route>//static/foo and stopped rendering.
The network tab shows 302 for those.

@alexfmpe

Copy link
Copy Markdown
Contributor

Reproduced same issue in the skeleton under ob run, but without the 302 (looks like that's only from nginx in ob deploy).
Oddly, http://localhost:8000//static/obelisk.jpg" seems to cause snap to return 200 OK even though the response has no content and we get no image

@ali-abrar

Copy link
Copy Markdown
Member

How are you generating the <route>/static/foo link? When I ob run, the image tag looks like this: <img data-ssr="" src="/static/obelisk.jpg">

@alexfmpe

alexfmpe commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

How are you generating the /static/foo link?

Oh, well I'm not, I meant locally on ob run (pre #930).
obeliska

@alexfmpe

alexfmpe commented Jun 2, 2022

Copy link
Copy Markdown
Contributor

Hmm actually I might have been confused. I can't reproduce the 200 anymore.

@madeline-os
madeline-os marked this pull request as ready for review June 18, 2022 18:37
@madeline-os

Copy link
Copy Markdown
Collaborator Author

I've determined that this is an issue for Android development. An asset referenced in an Android app will not be found unless the extra slash is dropped.

@alexfmpe

Copy link
Copy Markdown
Contributor

Huuuh something's fishy about my last two comments, it seems like I was getting my wires crossed with #930, hence the obeliska.jpg typo.

How are you generating the /static/foo link?

Hmm actually I might have been confused. I can't reproduce the 200 anymore.

On current develop,

--- a/skeleton/frontend/src/Frontend.hs
+++ b/skeleton/frontend/src/Frontend.hs
@@ -1,3 +1,4 @@
+{-# LANGUAGE LambdaCase #-}
 {-# LANGUAGE OverloadedStrings #-}
 {-# LANGUAGE TemplateHaskell #-}
 
@@ -42,7 +43,10 @@ frontend = Frontend
         ^. js ("skeleton_lib" :: T.Text)
         ^. js1 ("log" :: T.Text) ("Hello, World!" :: T.Text)
 
-      elAttr "img" ("src" =: $(static "obelisk.jpg")) blank
+      getConfig "common/route" >>= \case
+        Nothing -> blank
+        Just r -> do
+          elAttr "img" ("src" =: (T.decodeUtf8 r <> "/" <> $(static "obelisk.jpg"))) blank

leads to
src

200

whereas before #831 the same change gives me a working image and
old-static

@alexfmpe

alexfmpe commented Aug 14, 2022

Copy link
Copy Markdown
Contributor

Was this closed because the new behavior is being adopted or?

@madeline-os

Copy link
Copy Markdown
Collaborator Author

Yes. I noticed that the mobile infra already had a provision for dropping the slash: https://github.com/reflex-frp/reflex-dom/blob/develop/reflex-dom/java/org/reflexfrp/reflexdom/MainWidget.java#L161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants