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

fix: render image in absolute path #1868

Closed

Conversation

dandrade-meli
Copy link

@dandrade-meli dandrade-meli commented Aug 23, 2022

Summary

This PR documents support for including images with absolute paths and fixes a small bug that made it impossible to use.
This avoids the need to use ../../ to get to the root path.
Enable to fix bugs #850, #415

Example of use:

![example-logo](:_images/example/example-logo.png)
![example-logo](//_images/example/example-logo.png)
docsify
├── _images
│   └── example
│       └── example-logo.png
└── example
    ├── _coverpage.md | ![example-logo](:_images/example/example-logo.png)
    ├── _sidebar.md
    └── README.md
_coverpage.md
_sidebar.md
index.html
README.md

in /core/render/compiler/image.js which is responsible for generating the html img, there is a check if the href is not absolute.

if (!isAbsolutePath(href)) {
   url = getPath(contentBase, getParentPath(router.getCurrentPath()), href);
}

the isAbsolutePath function uses the following regex to do this check

/(:|(\/{2}))/g

what this regex does:
if the url contains : or //, then this path is absolute.

but when we used this strategy, adding the : or //, the image was not rendered because in the generated img tag the path kept the : or //

<img src=":_images/example/example-logo.png" data-origin=":_images/example/example-logo.png" alt="" title="">
<img src="//_images/example/example-logo.png" data-origin="//_images/example/example-logo.png" alt="" title="">

and to fix this bug I applied a simple replace to remove // and : by ``

<img src="_images/example/example-logo.png" data-origin="//_images/example/example-logo.png" alt="" title="">
<img src="_images/example/example-logo.png" data-origin="//_images/example/example-logo.png" alt="" title="">

What kind of change does this PR introduce?

Bugfix
Feature
Docs

For any code change,

  • Related documentation has been updated if needed
  • Related tests have been updated or tests have been added

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Related issue, if any:

#850
#415

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

@vercel
Copy link

vercel bot commented Aug 23, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
docsify-preview ✅ Ready (Inspect) Visit Preview Sep 1, 2022 at 9:48AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 23, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit e9fe6a9:

Sandbox Source
docsify-template Configuration

Copy link
Member

@Koooooo-7 Koooooo-7 left a comment

Choose a reason for hiding this comment

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

Hi @dandrade-meli , thx for ur investigation and input for this !
This change seems trick to match the regex of absolute condition tho.

I think maybe we should have a config such as :absolute to indicate an absolute path, it can get rid of our own isAbsolutePath check. and I could have a patch on it asap.
cc @docsifyjs/reviewers wdyt.

@jhildenbiddle
Copy link
Member

Let's identify the behavior we want first without worrying about implementation details. I think we also need to consider if/how Docsify's relativePath configuration option weighs into this discussion.

Given the following Docsify site structure:

[Root]
└── dir
    └── FILE.md
_sidebar.md
index.html
README.md

I believe Docsify's HTML output for /dir/FILE.md image URLs should be as follows:

Markdown URL Proposed output
relativePath:false
Proposed output
relativePath:true
current output
/img.png /img.png dir/img.png /dir/img.png
img.png img.png dir/img.png /dir/img.png
../img.png ../img.png ../img.png /dir/../img.png
/dir/img.png /dir/img.png dir/img.png /dir/img.png
../dir/img.png ../dir/img.png dir/img.png /dir/../dir/img.png

The table can be a bit confusing but the rules required to match the proposed output are simple:

  1. When relativePath:false (default), do not modify the markdown URL
  2. When relativePath:true...
    • Make URLs relative to the file context if the URL resolves to target within the file context. If the URL resolves to a target outside of the file context, do not modify the URL
    • Output relative URLs instead of absolute URLs. This allows Docsify sites contained in subdirectories to function properly

Thoughts?

@Koooooo-7
Copy link
Member

Let's identify the behavior we want first without worrying about implementation details. I think we also need to consider if/how Docsify's relativePath configuration option weighs into this discussion.

Yea, If the relativepath config is the highest rule, what the users do on paths should follow this config, then things should work well. Unfortunately, users wanna out of the rule and they can manually set one or more specific paths to absolute.
So, about this solution is based on :
A. Force user abide by the rules of the game.
B. Change the rule and left a back door.

Personally, I think it is fine to give users more sugars and tricks if the change would not break a big thing.

@dandrade-meli
Copy link
Author

Let's identify the behavior we want first without worrying about implementation details. I think we also need to consider if/how Docsify's relativePath configuration option weighs into this discussion.

Yea, If the relativepath config is the highest rule, what the users do on paths should follow this config, then things should work well. Unfortunately, users wanna out of the rule and they can manually set one or more specific paths to absolute. So, about this solution is based on : A. Force user abide by the rules of the game. B. Change the rule and left a back door.

Personally, I think it is fine to give users more sugars and tricks if the change would not break a big thing.

I believe the prefix :absolute idea adds the possibility without breaking the default behavior.

@jhildenbiddle
Copy link
Member

jhildenbiddle commented Sep 19, 2022

TL;DR:


I believe the prefix :absolute idea adds the possibility without breaking the default behavior.

It could, but...

  1. Docsify's path handling is problematic and/or broken in multiple scenarios, not just this one. The issue this PR attempts to solve is just one symptom of larger problems that need to be addressed.

    I don't mean that to sound critical of this PR as I appreciate the effort and recognize the need for a fix. My intention is only to call attention to the larger problems first so we can determine the best path forward.

    I've created a separate issue that explains the path-related issues in more detail and provides solutions here: Bug: Docsify's path handling is problematic (and broken in some scenarios) #1891. It can be a lot to digest, but the summary is simply that if we make path handling 1) consistent and 2) behave as expected (based on how they work everywhere else) by default, additional configuration options and custom markdown syntax will not be needed by the overwhelming majority of users.

    For perspective, here is a table demonstrating the current path-related issues:

    CleanShot 2022-09-19 at 15 08 31@2x

  2. Images are not the only resource that Docsify makes inaccessible via absolute path. The same issue affects embedded content (path is relative to the page route) and markdown pages in a nested site (path is relative to index.html). If we are going to add an :absolute attribute for image, we should do the same for other elements as well.

  3. The preferred behavior for an :absolute attribute can change based on the scenario. For example, assuming :absolute means "relative to the top-level of the domain or directory hierarchy" works here:

    [Root]
    ├─ assets
    │  └─ image.png
    ├─ dir
    │  └─ page.md
    ├─ index.html
    └─ README.md
    
    <!-- Works on README.md or page.md -->
    
    ![](/assets/image.png ':absolute')
    <img src="/assets/image.png">

    But the same behavior does not work here:

    [Root]
    └─ docs
       ├─ assets
       │  └─ image.png
       ├─ dir
       │  └─ page.md
       ├─ index.html
       └─ README.md
    

    Users could add /docs/ to their absolute path:

    <!-- Works on README.md or page.md -->
    
    ![](/docs/assets/image.png ':absolute')
    <img src="/docs/assets/image.png">

    But it is not unreasonable to assume or want "absolute" to mean "relative to the top-level of my site". In order to accomodate both scenarios, the attribute should accept a value. Renaming the attribute to :basepath makes sense to me since this is the purpose served by the attribute:

    ![](image.png ':basepath=/assets/')
    ![](image.png ':basepath=/assets/docs')
    <img src="/assets/image.png">
    <img src="/docs/assets/image.png">

@Koooooo-7: Personally, I think it is fine to give users more sugars and tricks if the change would not break a big thing.

I believe the goal should be for Docsify to be as intuitive as possible. Docsify's path handling is not intuitive. This is the result of both bugs and questionable design decisions made long ago.

These issues aren't obvious or common because they only present themselves is less-common scenarios (which is why they've survived this long), but when they do present themselves it's a mess. Adding configuration options and custom markdown syntax to get out of the mess instead of just fixing the issues that cause the mess is just avoiding the problem and adding more noise, both of which make Docsify less intuitive and less enjoyable for users.

FWIW, I have no issues adding configuration options or custom markdown syntax to to make Docsify more capable or enjoyable to use. I'm okay adding an :absolute-like feature so long as long as it makes sense in the big picture (i.e., after we fix the larger issues).

@Koooooo-7
Copy link
Member

Koooooo-7 commented Oct 4, 2022

close via #1891.
@dandrade-meli thx for your input !
If we finish the path issue, we could reopen it if needs.

@Koooooo-7 Koooooo-7 closed this Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants