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: genIndex error for search #1933

Merged
merged 10 commits into from
Jan 3, 2023
Merged

Conversation

wangliang181230
Copy link
Contributor

@wangliang181230 wangliang181230 commented Dec 6, 2022

Summary

What kind of change does this PR introduce?

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:

Tested in the following browsers:

  • Chrome
  • Firefox
  • Safari
  • Edge
  • IE

1. url is wrong if multiple contents are on the same site, example: `https://xxxxx/docs/` and `https://xxxxx/blog/`
2. when the `token.text` is undefined, `handlePostContent` is undefined too at line 216
…ires, this plugin can still operate normally
@vercel
Copy link

vercel bot commented Dec 6, 2022

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

Name Status Preview Updated
docsify-preview ✅ Ready (Inspect) Visit Preview Jan 3, 2023 at 6:54AM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 6, 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 f41ee5a:

Sandbox Source
docsify-template Configuration

@@ -128,9 +142,7 @@ export function genIndex(path, content = '', router, depth) {
token.text = getTableData(token);
token.text = getListData(token);

index[slug].body = index[slug].body
? index[slug].body + token.text
: token.text;
Copy link
Contributor Author

@wangliang181230 wangliang181230 Dec 6, 2022

Choose a reason for hiding this comment

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

index[slug].body is always undefined, see the line 122

120  if (!index[slug]) {
121    index[slug] = { slug, title: '', body: '' };
122  } else if (index[slug].body) {
       ......
127  } else {
       ...... //index[slug].body is always undefined
     }

Copy link
Contributor Author

@wangliang181230 wangliang181230 Dec 14, 2022

Choose a reason for hiding this comment

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

When the token.text is undefined, it will make the index[slug].body also undefined.
So I change the token.text to token.text || ''.

Copy link
Contributor Author

@wangliang181230 wangliang181230 Dec 14, 2022

Choose a reason for hiding this comment

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

The code index[slug].body ? index[slug].body + token.text : is redundant, and it is not the cause of the bug.

Copy link
Member

Choose a reason for hiding this comment

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

nice point.


if (isExpired) {
if (!INDEXS) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When manually delete docsify.search.index and keep docsify.search.expires, this plugin can still operate normally

Copy link
Member

Choose a reason for hiding this comment

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

The user should know what he does when he gonna manually delete index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to reduce a manual operation step during the test, haha.

Copy link
Member

Choose a reason for hiding this comment

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

Just to reduce a manual operation step during the test, haha.

If user wanna break it, he should break it.

Copy link
Contributor Author

@wangliang181230 wangliang181230 Dec 19, 2022

Choose a reason for hiding this comment

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

Then this change is not necessary, is it?
If not, I'll revert this code.

Copy link
Member

Choose a reason for hiding this comment

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

Then this change is not necessary, is it? If not, I'll revert this code.

yep. personally, I think we do not need handle this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

slug = '#/' + slug.substring(5);
}
slug = pathname + (flag ? '/' : '') + slug;

Copy link
Member

Choose a reason for hiding this comment

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

What does those changes for ? could you plz give a sample and add verify test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When my website has multiple paths, it can prevent 404 error.
For example:
http://xxxxxx/A/
http://xxxxxx/B/
When searching on page A and accessing page B, 404 will appear.

Copy link
Member

Choose a reason for hiding this comment

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

When my website has multiple paths, it can prevent 404 error. For example: http://xxxxxx/A/ http://xxxxxx/B/ When searching on page A and accessing page B, 404 will appear.

I see, maybe u can take a look on this namespace configs here.

Copy link
Contributor Author

@wangliang181230 wangliang181230 Dec 19, 2022

Choose a reason for hiding this comment

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

I see, maybe u can take a look on this namespace configs here.

Should I configure it like this?

{
    search: [
        '/A',
        '/B'
    ]
}

or

{
    search: {
        paths: [
            '/A',
            '/B'
        ]
    }
}

Copy link
Member

@Koooooo-7 Koooooo-7 Jan 3, 2023

Choose a reason for hiding this comment

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

I guess you need distinguish those namespace for different paths (sites), may could refer to our site with multi langs nav . if you deploy multi sites in same domain, the namespace config should be in each site's index.html search config.

      // Use different indexes for path prefixes (namespaces).
      // NOTE: Only works in 'auto' mode.
      //
      // When initialiazing an index, we look for the first path from the sidebar.
      // If it matches the prefix from the list, we switch to the corresponding index.
      pathNamespaces: ['/zh-cn', '/ru-ru', '/ru-ru/v1'],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I will try it.

@wangliang181230 wangliang181230 changed the title fix: fix search.js two BUGs: 404 and handlePostContent is undefined fix: fix handlePostContent is undefined Jan 3, 2023
@wangliang181230
Copy link
Contributor Author

PTAL

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.

LGTM.

@Koooooo-7 Koooooo-7 requested a review from a team January 3, 2023 07:13
@sy-records sy-records changed the title fix: fix handlePostContent is undefined fix: genIndex error for search Jan 3, 2023
@sy-records sy-records merged commit a8f9fc1 into docsifyjs:develop Jan 3, 2023
@wangliang181230 wangliang181230 deleted the bugfix-1 branch January 3, 2023 07:47
sy-records pushed a commit that referenced this pull request Jun 24, 2023
@sy-records sy-records mentioned this pull request Jun 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants