favicon prefixes href with leading slash (/) when url is absolute

[techfg]

What version of starlight are you using?

0.29.2

What version of astro are you using?

4.16.10

What package manager are you using?

npm

What operating system are you using?

linux

What browser are you using?

chrome

Describe the Bug

When favicon configuration parameter contains an absolute url, the generated link in head contains a leading slash.

integrations: [
    starlight({
	favicon: 'https://starlight.astro.build/favicon.svg',

Absolute URLs are valid for favicon so I believe one of the following should be applied:

1. If Starlight requires a local resource, configuration validation should fail if url is not relative
2. Starlight should allow absolute urls and not prefix with leading slash

Link to Minimal Reproducible Example

https://stackblitz.com/edit/withastro-starlight-15r2cj

Participation

• I am willing to submit a pull request for this issue.

[SnowDingo]

@techfg, Thank you for your report!👍

I have spent couple days trying to understand the codebase of starlight, especially around the schemas.
After several try and errors, I discovered that the 2nd solution is the easiest and safest options to implement.

After all the experiments, I made a PR that fixes this issue↓
#2651

Best Regards,
SnowDingo

[techfg]

Hi @SnowDingo -

Thanks for reviewing this and I agree, I believe the 2nd option is likely best since favicon can be absolute or relative url and therefore I feel Starlight should support both.

Unfortunately, in taking a look at the #2651, it appears that if the url starts with https? that schema validation will fail meaning absolute would not work. I believe in order to allow for absolute or relative url, this code would need to be modified.

Would recommend to wait to hear from a maintainer on which approach they would like Starlight to take on this one and once decided, possibly you could adjust the PR you submitted based on their feedback.

Thanks!

[SnowDingo]

Dear @techfg.
Yes, I believe it's true that we have to hear from the starlight community. I implemented the 1st option in my PR since it is the simplest option, but I can try to remake it so that the second option is implemented.
However, although I spent another hour figuring out why it adds / on the front, I couldn't find the code that adds the / as the prefix. Additionally, I found a comment on user-config.ts saying,
/** The default favicon for your site which should be a path to an image in the public/directory. */.
So I'm not sure which one is the best option.

Best Regards,
SnowDingo

[techfg]

Hi @SnowDingo -

Yes, agreed, not sure which option here is best and why I think waiting for a maintainer to provide input & a decision makes sense before doing any future work on a potential PR. The decision may be to leave things as-is, implement option 1 (require a relative url only and enforce in schema validation) or option 2 (allow absolute & relative and handle the / accordingly). My recommendation would be Option 2 since absolute urls in favicon are valid but from a Starlight perspective specifically, it may be determined otherwise.

absolute would not work. I believe in order to allow for absolute or relative url, this code would need to be modified.

Regarding where in the code the / gets appended, I mentioned in my previous comment (see the "this code" link) where I believe the code would have to change to allow for absolute URLs. Specifically, the / is appended in the fileWithBase function but in the case of an absolute URL, fileWithBase should not be called.

[SnowDingo]

Hi @SnowDingo -

Yes, agreed, not sure which option here is best and why I think waiting for a maintainer to provide input & a decision makes sense before doing any future work on a potential PR. The decision may be to leave things as-is, implement option 1 (require a relative url only and enforce in schema validation) or option 2 (allow absolute & relative and handle the / accordingly). My recommendation would be Option 2 since absolute urls in favicon are valid but from a Starlight perspective specifically, it may be determined otherwise.

absolute would not work. I believe in order to allow for absolute or relative url, this code would need to be modified.

Regarding where in the code the / gets appended, I mentioned in my previous comment (see the "this code" link) where I believe the code would have to change to allow for absolute URLs. Specifically, the / is appended in the fileWithBase function but in the case of an absolute URL, fileWithBase should not be called.

Thank you for your idea @techfg!
I will try to implement the second option by looking deeper at the code.
Would it be possible to wait a little bit more so that I can finish up and re-submit a PR that implements the second option?

I've made changes to the base.ts code as the following and it seems to be working for both absolute and relative URL:

/** Get the a root-relative file URL path with the site’s `base` prefixed. */
export function fileWithBase(path: string) {
	path = stripLeadingSlash(path);
	return path ? base + '' + path : base;
}

[techfg]

@SnowDingo -

Yes, as mentioned, I would strongly encourage you to wait on any further PR updates until a maintainer has provided guidance on how absolute URLs should be handled.

Once a decision is made, if its determined to support absolute urls for favicon, the approach you shared will likely break all areas of the current code base that rely on fileWIthBase since they expect the / to be added after the base and before the path. Additionally, when the url is absolute, it would still prefix with base if there is one configured.

As I mentioned in prior comment, I think the best way to support absolute URLs would be to modify this code to not call fileWithBase at all if the favicon property value is an absolute URL leaving fileWithBase untouched. In short, if favicon is absolute url, its value is directly used for href attribute and if not absolute url, the value returned from fileWithBase as used. There may already be a utility function that exists that handles the absolute vs. relative url case in this way although I haven't looked closely to see if one exists.

[SnowDingo]

@SnowDingo -

Yes, as mentioned, I would strongly encourage you to wait on any further PR updates until a maintainer has provided guidance on how absolute URLs should be handled.

Once a decision is made, if its determined to support absolute urls for favicon, the approach you shared will likely break all areas of the current code base that rely on fileWIthBase since they expect the / to be added after the base and before the path. Additionally, when the url is absolute, it would still prefix with base if there is one configured.

As I mentioned in prior comment, I think the best way to support absolute URLs would be to modify this code to not call fileWithBase at all if the favicon property value is an absolute URL leaving fileWithBase untouched. In short, if favicon is absolute url, its value is directly used for href attribute and if not absolute url, the value returned from fileWithBase as used. There may already be a utility function that exists that handles the absolute vs. relative url case in this way although I haven't looked closely to see if one exists.

Ok. @techfg, I will make another PR that accepts absolute URLs.
From that information, I progressed a bit and created a new utility function that enables the Astro.js database to determine if the path is absolute or relative.

// A util function to determine if the path is relative or not.
export function absorrelative(path:string){
	if (/^https?:\/\//.test(path)) {
		return path;
	}else{
		// Relative URL
		return fileWithBase(path);
	}
}

This code didn't work, so I experimented a bit with the function itself. (i.g adding if true to the if statements and changing some of the fileBase methods to the util function I just created. As a result, I discovered that the fileWithBase function is called somewhere no matter the context of the if statement in the base.ts file, and that part of the code adds the unnecessary slash.

[techfg]

Hello @SnowDingo -

I appreciate your enthusiasm to provide a PR with a fix 😄 However, we still do not know what the starlight maintainers believe the expected behavior should be. Also, feedback on code/approach is better to have within a PR and not in an issue like what is occurring in this issue in recent comments because it takes the focus away from what the issue is and what the expected behavior should be.

As mentioned, I'd strongly encourage you to hold off on any further work until we hear from the maintainers on what the expected behavior should be:

1. Leave as-is
2. Do not allow absolute URLs by enforcing in configuration validation
3. Allow absolute and relative URLs (my recommendation)

Once they provide that input, if you update the PR you've submitted to align with the expected behavior, I'd be happy to provide input in the PR itself on how to correct any issues you may be having. For now, let's avoid any discussion on the code itself in this issue.

Thanks!

[SnowDingo]

Yes, it will be better to hold off on working on this issue right now. I talked to @delucis in Discord, and he also said to leave it as it is.
Thank you @techfg!!

[techfg]

Thanks for the update @SnowDingo!

@delucis - For when you have the time to review this issue, I think what your suggesting is to keep it relaxed meaning to allow relative or absolute, correct? Assuming yes (which would be recommendation FWIW), then there would be a fix needed to ensure that when absolute, / is not prefixed to the url applied to favicon link (this is what the OP of this issue is intended to identify/address). No rush on this, just wanted to clarify what you mentioned from the screenshot. Thanks!