Skip to content

The toc (with subMaxLevel) is generated twice when there's 2 embedded links in a page #1822

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

Closed
1 task done
julienw opened this issue Jun 22, 2022 · 2 comments · Fixed by #1824
Closed
1 task done
Labels
bug confirmed as a bug

Comments

@julienw
Copy link
Contributor

julienw commented Jun 22, 2022

Bug Report

Steps to reproduce

  1. Use this configuration:
      window.$docsify = {
        subMaxLevel: 2,
        loadSidebar: true
      };
  1. Use a _sidebar.md, it could be as simple as:
- [Home](/)
  1. add a file with 2 embedded files, such as:
# Hello

Hello world

## Title 1

Lorem ipsum

[insert something](file.mp4 ":include")

[insert something else](file.mp4 ":include")

You can also see it live in https://codesandbox.io/s/problem-with-docsify-4cyldv

What is current behaviour

The Toc is generated twice, but in a nested way. In the above example, this looks like it:

  • Home
    • Title 1
    • Hello
      • Title 1

See also this screenshot:
image

What is the expected behaviour

We have the TOC only once, this should look like this in this case:

  • Home
    • Title 1

This happens when there's only one embedded asset.

Other relevant information

  • Bug does still occur when all/other plugins are disabled?

  • Your OS: not relevant

  • Node.js version: not relevant

  • npm/yarn version: not relevant

  • Browser version: Firefox 103, but I also tried in Chrome stable, so I believei t's not relevant

  • Docsify version: 4.12.2

  • Docsify plugins: none

Please create a reproducible sandbox

https://codesandbox.io/s/problem-with-docsify-zl23lm

Mention the docsify version in which this bug was not present (if any)

I haven't tried earlier versions, it's the first time I try this.

@julienw
Copy link
Contributor Author

julienw commented Jun 22, 2022

I tried to debug, and I think the issue is that the 2 callbacks are called for each of the token:

cb({ token, embedToken });
if (++count >= step) {
cb({});
}

As a result, done here is called twice:

done(tokens);

And then this is called twice:

html = this.compiler.compile(tokens);
callback();

Which isn't usually a problem (except things may be rendered twice), but in the case of the sidebar toc, this causes the toc to be rendered twice with:

_self.toc.push(nextToc);

Then the duplicated toc is used in this routine:

subSidebar(level) {
if (!level) {
this.toc = [];
return;
}
const currentPath = this.router.getCurrentPath();
const { cacheTree, toc } = this;
toc[0] && toc[0].ignoreAllSubs && toc.splice(0);
toc[0] && toc[0].level === 1 && toc.shift();
for (let i = 0; i < toc.length; i++) {
toc[i].ignoreSubHeading && toc.splice(i, 1) && i--;
}
const tree = cacheTree[currentPath] || genTree(toc, level);
cacheTree[currentPath] = tree;
this.toc = [];
return treeTpl(tree);
}

Here only the first toc entry is removed, if it's a depth 1.

Therefore I can see these possible fixes:

  1. Remove all TOC entries with depth==1 when generating the sidebar. This is more a hacky quick fix than a longterm solution IMO, but this could at least fix this issue I believe. No that wouldn't work anyway, because the subheadings are also present, so the Toc would still be duplicated, but not nested.
  2. Make sure that done is called only once for each render. I don't understand enough the logic with step and count that are both incremented, so I don't know what's the intent here...

Hpoe this helps!

@julienw
Copy link
Contributor Author

julienw commented Jun 22, 2022

I think I understood the dance with the incrementations: when the token has a url, then the process is asynchronous, so the condition is there to ensure that we call the callback after everything has been fetched. But this doesn't work when the token is just some html, because in that case the process is synchronous. The fix is to make this case asynchronous as well. I think I can make a patch for that.

update: Studying the code some more, I decided to use the length of the embedTokens source instead of using step. This makes things a lot more predictable in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug confirmed as a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants