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

feat(theme-algolia): add option.replaceSearchResultPathname to process/replaceAll search result urls #8428

Merged
merged 11 commits into from Dec 22, 2022

Conversation

mellson
Copy link
Contributor

@mellson mellson commented Dec 12, 2022

Implemented solution

replaceSearchResultPathname added - doc: https://deploy-preview-8428--docusaurus-2.netlify.app/docs/search/#connecting-algolia

{
  // Optional: Replace parts of the item URLs from Algolia. Useful when using the same search index for multiple deployments using a different baseUrl. You can use regexp or string in the `from` param. For example: localhost:3000 vs myCompany.com/docs
  replaceSearchResultPathname: {
    from: '/docs/', // or as RegExp: /\/docs\//
    to: '/',
    },
}

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

We have an upcoming documentation site we're building at https://stately.ai/docs/. So we've set our baseUrl in the config to /docs/. However when we do our preview deploys, these are served from root /.

This setup makes it hard to use the search on the preview deploys because it indexes based on the site with a different baseUrl.

This PR adds the option to replace the item's URL that Algolia returns. So in our case we would like to replace /docs/ with /.

Test Plan

Test links

Deploy preview: https://deploy-preview-8428--docusaurus-2.netlify.app/

Related issues/PRs

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 12, 2022
@netlify
Copy link

netlify bot commented Dec 12, 2022

[V2]

Name Link
🔨 Latest commit 7847734
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63a48a09e7b3280008e3ecaf
😎 Deploy Preview https://deploy-preview-8428--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Dec 12, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟢 94 🟢 97 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 76 🟢 100 🟢 100 🟢 100 🟢 90 Report

@mellson mellson changed the title Make it possible to replace in the item URL Make it possible to replace in the returned Algolia item URL Dec 12, 2022
@mellson mellson changed the title Make it possible to replace in the returned Algolia item URL Make it possible to replace parts of the returned Algolia item URL Dec 13, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

👍 Agree this feature could be useful, and I would probably use it myself

Not 100% fan of the API as it's not very flexible and the naming doesn't feel right

If we can make it better how let's do this. Otherwise I won't block this PR too much because it's a useful but quite niche feature and we can do breaking changes later if needed.

website/docs/search.md Outdated Show resolved Hide resolved
@slorber slorber added pr: new feature This PR adds a new API or behavior. to backport This PR is planned to be backported to a stable version of Docusaurus labels Dec 15, 2022
@mellson
Copy link
Contributor Author

mellson commented Dec 15, 2022

👍 Agree this feature could be useful, and I would probably use it myself

Not 100% fan of the API as it's not very flexible and the naming doesn't feel right

If we can make it better how let's do this. Otherwise I won't block this PR too much because it's a useful but quite niche feature and we can do breaking changes later if needed.

Thank you for your feedback, I agree with all your suggestions and have implemented them in the latest commit.

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Can you add this to our localhost + deploypreview? That would make sure that we use our own new feature and we could be sure it works end 2 end with the preview.

Example search result:

I guess this should work: {from: "/docs/next", to: "/docs"}

website/versioned_docs/version-2.0.1/search.md Outdated Show resolved Hide resolved
@mellson
Copy link
Contributor Author

mellson commented Dec 16, 2022

LGTM 👍

Can you add this to our localhost + deploypreview? That would make sure that we use our own new feature and we could be sure it works end 2 end with the preview.

Example search result:

I guess this should work: {from: "/docs/next", to: "/docs"}

That makes sense 👍🏻 I've updated the config to use this in dev- and preview mode. And made it possible to provide a regexp or a string in the from param.

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Not super fan of the getRegexpOrString.

I'd prefer either a better solution or to have it removed and just support regexp strings.

I'm a bit afraid this might lead to confusing behaviors when using special chars in from including [({&+... I'd rather let the user decide explicitly if they want to use a RexExp or a simple string replacement

* @param possibleRegexp string that is possibly a regex
* @returns a Regex if possible, otherwise the string
*/
export function getRegexpOrString(possibleRegexp: string): RegExp | string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤔 I don't really like this implementation and would prefer to just support regexp. Adding unit tests would likely reveal the problematic cases.

My idea was more:

  • config accepts both string + RegExp
  • config validation normalizes this as a serializable RegExp string
  • client-side always use new RexExp()

The config normalization can be something like:

if (from instanceof string) {
  return escapeRegexp(from);
} else if (from instanceof RegExp) {
 return from.source;
} else {
  throw new Error('unexpected")
}

=> always return a valid Regexp string source

Does it make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense; I wasn't terribly happy with getRegexpOrString either. I've tried to address your suggestions in the latest commit, hope I got it right this time 😅

@slorber
Copy link
Collaborator

slorber commented Dec 22, 2022

I'm going to push some little refactors in the PR before merging so please don't commit 🤪

@slorber slorber changed the title Make it possible to replace parts of the returned Algolia item URL feat(theme-algolia): add option.replaceSearchResultPathname to process/replaceAll search result urls Dec 22, 2022
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks 👍

Looks like it works fine on both the search bar and search page, on localhost + deploy preview

@slorber slorber merged commit 19ba0ff into facebook:main Dec 22, 2022
@mellson mellson deleted the mellson/add-replace-in-item-url-option branch December 22, 2022 17:42
slorber added a commit that referenced this pull request Jan 26, 2023
@slorber slorber added backported This PR has been backported to a stable version of Docusaurus and removed to backport This PR is planned to be backported to a stable version of Docusaurus labels Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR has been backported to a stable version of Docusaurus CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants