trunk icon indicating copy to clipboard operation
trunk copied to clipboard

Added an additional prefix case for public_url to allow relative paths.

Open Earthmark opened this issue 3 years ago • 5 comments

Checklist

  • [X] Updated CHANGELOG.md describing pertinent changes.
  • [ ] Updated README.md with pertinent info (may not always apply). - NA
  • [ ] Updated site content with pertinent info (may not always apply). - NA
  • [X] Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

With this the public_url field will remain relative if the path starts with ./

This supports cases with CDN's such as itch.io which requires relative paths to host static files.

Existing users that use api/taco/ will still have the url coerced to /api/taco/. But if they specify ./api/taco/ then ./api/taco/ is used in the output. .bin/api is still coerced to /.bin/api/, paths must exactly start with ./.

It may be better to omit the ./ in the output, let me know if that's wanted.

It may be valid to extend the check to be !(val.starts_with('/') || val.starts_with("./") || val == "."), as trunk build --public-url "." feels very natural to write, but results in /./ being emitted.

Earthmark avatar Apr 17 '22 03:04 Earthmark

I forgot to tag: this resolves #165 and #320

~~As an additional note, this is an alternative change that would allow trunk build --public-url "."~~

/// Ensure the given value for `--public-url` is formatted correctly.
pub fn parse_public_url(val: &str) -> String {
    if val == "." {
        return "".into();
    }
    // If val don't start with a / (absolute) or . (relative) path, default to absolute.
    let prefix = if !(val.starts_with('/') || val.starts_with("./")) {
        "/"
    } else {
        ""
    };
    let suffix = if !val.ends_with('/') { "/" } else { "" };
    format!("{}{}{}", prefix, val, suffix)
}

EDIT: Keeping the ./ at the start is required, ignore this proposed amendment. Adding the || val == "." like in the initial PR would work.

Earthmark avatar Apr 17 '22 04:04 Earthmark

Hm, this may not work well for cargo serve, as it is probably not supported by the Axum router to have relative urls.

Will do a bit testing locally. My guess is we have to do an additional step to transform the url, before we use it in the serve command.

dnaka91 avatar May 01 '22 10:05 dnaka91

@Earthmark looks like @dnaka91's comment hasn't been addressed. Please advise.

thedodd avatar Jun 30 '22 02:06 thedodd

This would be good for uploading sites to IPFS also. (Personally I only need it working well for trunk build for when it's being distributed rather than when being run locally)

gilescope avatar Aug 16 '22 07:08 gilescope

Even if it doesn't work with trunk serve, it would be very helpful for trunk build as it's only then that typically we change the public url.

gilescope avatar Sep 05 '22 07:09 gilescope

For anyone else looking to sed this seems to do the trick to add a . infront of the /: cat ./dist/index.html | sed "s@'/@'./@g" | sed 's@"/@"./@g'

gilescope avatar Nov 09 '22 09:11 gilescope

I totally forgot about this, my apologies there. I think the sed approach is probably good enough for most use cases. This is particularly for a CI system so trunk could produce a correct artifact, but I'm not sure what the effect will be on the serve path. So, I'll close this for now, but if it becomes an issue that I need to solve later I'll certainly push an update back up the pipe.

Earthmark avatar Nov 17 '22 01:11 Earthmark

I don't know that this patch specifically is the right way to go about this, but is there some way to accomplish something like this change?

I have a use case where I want to produce files via trunk build that any user can upload to any webhost, anywhere. The "anywhere" part is important, because the files are specifically intended to be shared. And I can't expect an average user to know about the absolute path they're uploading to, I need it to just work. Relative URLs make that work much more easily.

For what it's worth, Jekyll went through a similar transition in the early days. It used to be that you needed to specify the baseurl of your site if it wasn't hosted under /, which was a pain and ended up being very brittle in practice. Since they changed to relative URLs I've hardly ever had to think about it.

elliottslaughter avatar Dec 16 '22 07:12 elliottslaughter

Exactly. I don't understand why relative paths is not the default...

gilescope avatar Dec 18 '22 14:12 gilescope