-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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(v2): use swizzled SearchPage component if any #3721
Conversation
): string | undefined { | ||
const swizzledComponentPath = path.resolve( | ||
process.cwd(), | ||
'src', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the circular dependency issue, I can't use a constant from core (import {SRC_DIR_NAME} from '@docusaurus/core/lib/constants';
) here, so I had to use a hard-coded string src
.
lerna WARN ECYCLE Dependency cycles detected, you should fix these!
lerna WARN ECYCLE @docusaurus/utils-validation -> @docusaurus/utils -> @docusaurus/core -> @docusaurus/utils-validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes already had this issue.
I think we could have a @docusaurus/constants
project to solve this once for all, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to create new package that will include only constants, maybe we can add something else to it? But if there are no other options, then I think it's worth moving the constants into separate package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit annoying to have too many packages that do very little, but afaik it's the only clean solution to handle this properly.
Even in core, I had to split in server-side constants / client-side constants, and needed some duplication, see also packages/docusaurus/src/client/exports/constants.ts
Deploy preview for docusaurus-2 ready! Built with commit 07b36ed |
@@ -29,7 +29,10 @@ function theme(context) { | |||
baseUrl, | |||
siteConfig: {title, url, favicon}, | |||
} = context; | |||
const pagePath = path.resolve(__dirname, './theme/SearchPage/index.js'); | |||
const pageComponent = './theme/SearchPage/index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just wondering, wouldn't using @theme/SearchPage
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because we need resolved component path, so it will be used with fs
further.
Motivation
In PR #3280 it was allowed to swizzle
SearchPage
component, but newly swizzled component was not taken into account when building site.This PR fixes #3379, after that if the search page component has been changed it will be used instead of initial one.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Try swizzling the component, making changes to it, starting the site, and making sure the swizzled component is actually being used.
Related PRs
(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)