Skip to content

fix: genIndex error for search - #1933

Merged
sy-records merged 10 commits into
docsifyjs:developfrom
wangliang181230:bugfix-1
Jan 3, 2023
Merged

fix: genIndex error for search#1933
sy-records merged 10 commits into
docsifyjs:developfrom
wangliang181230:bugfix-1

Conversation

@wangliang181230

@wangliang181230wangliang181230 commented Dec 6, 2022

Copy link
Copy Markdown
Contributor

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
@vercel

vercelBot commented Dec 6, 2022

Copy link
Copy Markdown

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

NameStatusPreviewUpdated
docsify-preview✅ Ready (Inspect)Visit PreviewJan 3, 2023 at 6:54AM (UTC)

@codesandbox-ci

codesandbox-ciBot commented Dec 6, 2022

Copy link
Copy Markdown

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:

SandboxSource
docsify-templateConfiguration


index[slug].body = index[slug].body
? index[slug].body + token.text
: token.text;

@wangliang181230wangliang181230Dec 6, 2022

Copy link
Copy Markdown
ContributorAuthor

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

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

@wangliang181230wangliang181230Dec 14, 2022

Copy link
Copy Markdown
ContributorAuthor

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 || ''.

@wangliang181230wangliang181230Dec 14, 2022

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice point.

Comment threadsrc/plugins/search/search.js Outdated
INDEXS = isExpired ? null : JSON.parse(localStorage.getItem(indexKey));

if (isExpired) {
if (!INDEXS) {

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@wangliang181230wangliang181230Dec 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

done

Comment threadsrc/plugins/search/search.js Outdated
slug = '#/' + slug.substring(5);
}
slug = pathname + (flag ? '/' : '') + slug;

Copy link
Copy Markdown
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
Copy Markdown
ContributorAuthor

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
Copy Markdown
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.

@wangliang181230wangliang181230Dec 19, 2022

Copy link
Copy Markdown
ContributorAuthor

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'
]
}
}

@Koooooo-7Koooooo-7Jan 3, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
ContributorAuthor

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.

@wangliang181230wangliang181230 changed the title fix: fix search.js two BUGs: 404 and handlePostContent is undefinedfix: fix handlePostContent is undefinedJan 3, 2023
@wangliang181230

Copy link
Copy Markdown
ContributorAuthor

PTAL

@Koooooo-7Koooooo-7 left a comment

Copy link
Copy Markdown
Member

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 teamJanuary 3, 2023 07:13
@sy-recordssy-records changed the title fix: fix handlePostContent is undefinedfix: genIndex error for searchJan 3, 2023
@sy-records
sy-records merged commit a8f9fc1 into docsifyjs:developJan 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-recordssy-records mentioned this pull request Jun 24, 2023
@sy-recordssy-records mentioned this pull request May 5, 2025
@sy-recordssy-records mentioned this pull request Jul 17, 2026
Sign up for freeto 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

@wangliang181230@Koooooo-7@sy-records