denoland / dotland

[Archived] deno.land website
https://deno.land
MIT License
957 stars 626 forks source link

Relative URLs in readme markdown starting with `./` render broken #2060

Closed jaydenseric closed 2 years ago

jaydenseric commented 2 years ago

Relative URLs in readme markdown starting with ./ should link to files within the package, but incorrectly deno.land renders them as if they are deno.land/x packages.

Here is an example readme with broken relative URLs (the top logo SVG, and various links to package modules):

https://deno.land/x/ruck@v1.0.0

Similar to a past issue (that was apparently fixed) for relative URLs that don't start with ./:

https://github.com/denoland/dotland/issues/765

jaydenseric commented 2 years ago

It's working when the readme is viewed directly, e.g.

https://deno.land/x/ruck@v1.1.0/readme.md

But not when the main package page displays the readme contents at the bottom:

https://deno.land/x/ruck@v1.1.0

M4rteaga commented 2 years ago

I'd like to help out with this issue.

M4rteaga commented 2 years ago

It seems that in DisplayFile.tsx when rendering a markdown file the baseUrl prop is missing.

https://github.com/denoland/dotland/blob/46462c8a3c3ca830d49dfa110f26d3231231d334/components/FileDisplay.tsx#L114-L127

However in Markdown.tsx the baseUrl prop is optional but later is used to call the gfm function.

https://github.com/denoland/dotland/blob/46462c8a3c3ca830d49dfa110f26d3231231d334/components/Markdown.tsx#L4-L16

As part of the solution, I tried to add the baseUrl prop, but it still doesn't work

baseUrl = {props.baseURL}

Conclusion

It seems that the problem is related to the gfm function that treats the markdown. This is a dependency linked to crowlKats/deno-gfm repo. There are some custom changes to the links. A think here is where the problem is.

https://github.com/crowlKats/deno-gfm/blob/663b1cc026ac881b2fe8928f157db0126ee08dba/mod.ts#L29-L34

 link(href: string, title: string, text: string) {
    if (href.startsWith("#")) {
      return `<a href="${href}" title="${title}">${text}</a>`;
    }
    return `<a href="${href}" title="${title}" rel="noopener noreferrer">${text}</a>`;
  }
M4rteaga commented 2 years ago

One possible solution is to add the baseUrl prop to the markdown component in DisplayFile.tsx

case 'markdown': {
    return (
        <div class="px-4">
            <Markdown
                source={
                    props.stdVersion === undefined
                        ? props.raw!
                        : props.raw!.replace(
                                /\$STD_VERSION/g,
                                props.stdVersion ?? ''
                          )
                }
                baseUrl={props.baseURL}
            />
        </div>
    );
}

Additionally, add this case to the custom link function of the render (gfm) dependency library (deno-gfm)

if (href.startsWith('.') && this.options.baseUrl) {
    return `<a href="${`${this.options.baseUrl}/${href}`}" title="${title}" rel="noopener noreferrer">${text}</a>`;
}

it should look like this:

link(href: string, title: string, text: string) {
        if (href.startsWith('#')) {
            return `<a href="${href}" title="${title}">${text}</a>`;
        }
        if (href.startsWith('.') && this.options.baseUrl) {
            return `<a href="${`${this.options.baseUrl}/${href}`}" title="${title}" rel="noopener noreferrer">${text}</a>`;
        }
        return `<a href="${href}" title="${title}" rel="noopener noreferrer">${text}</a>`;
    }
stabai commented 2 years ago

Any update on this? Every deno.land/x package's README file is currently broken by this when clicking any link to another file in the repo.

I'm not sure that any of the above solutions fully take into account the fact that https://deno.land/x/foo/README.md works while https://deno.land/x/foo does not. The reason is that https://deno.land/x/foo is accessing the folder foo, but because the server is using URL rewriting to remove the trailing slash, the browser can't tell. Rather than thinking we're reading the contents of the deno.land/x/foo folder, it thinks w'ere reading a file named foo from within the deno.land/x folder.

https://deno.land/x/foo/README.md  +  ./deps.ts  =  https://deno.land/x/foo/deps.ts ✅
https://deno.land/x/foo            +  ./deps.ts  =  https://deno.land/x/deps.ts     ❌
https://deno.land/x/foo/           +  ./deps.ts  =  https://deno.land/x/foo/deps.ts ✅

Without the trailing slash to inform the browser that foo is a folder rather than a file, the base URL needs to be different in that case than in all other cases.

Note: if the server's URL rewriting were reconfigured to force trailing slashes for folders instead of removing them (e.g. rewrite https://deno.land/x/foo to https://deno.land/x/foo/), this issue would disappear. I'm not sure if this would be an easy or good solution, just throwing it out there.

stabai commented 2 years ago

I might be misunderstanding the code, but it looks like maybe the operation done here also needs to be done on the baseURL passed to FileDisplay:

https://github.com/denoland/dotland/blob/fe5cec62951471a242a719a386fd20d33e4c5de1/components/DocView.tsx#L43