-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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(core): do not apply theme-init alias to user component #5983
Conversation
✔️ [V2] 🔨 Explore the source changes: 0040819 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61aa5796a35aeb0008042fee 😎 Browse the preview: https://deploy-preview-5983--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5983--docusaurus-2.netlify.app/ |
Size Change: 0 B Total Size: 656 kB ℹ️ View Unchanged
|
/cc @lex111 Since you implemented this alias in the first place—I hope my understanding is correct? |
@@ -23,21 +23,23 @@ Object { | |||
"@docusaurus/useIsBrowser": "../../../../client/exports/useIsBrowser.ts", | |||
"@generated": "../../../../../../..", | |||
"@site": "", | |||
"@theme-init/PluginThemeComponentOverridden": "pluginThemeFolder/PluginThemeComponentOverridden.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.
PluginThemeComponentOverriddenByUser
is no longer getting a @theme-init
alias because it's not provided by two themes, only by the user and one theme.
@Josh-Cena yes, that's right. It really isn't the easiest/most obvious thing to understand and describe in words, so thank you for documenting it! |
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.
Thanks, LGTM 👍
The doc is clearer indeed :)
Added an empty theme/CodeBlock
wrapper on our site to dogfood
Motivation
This PR essentially reverts #3485.
#3485 makes user theme components have the
@theme-init
alias as well, however that's not the intention of this alias. That PR was only created because of the lack of knowledge of@theme-original
at that time.@theme-init
is supposed to load the "initial version" of this component, when it's first provided. I made a contrived example to illustrate the bug (sorry for not filing an issue or creating a repro, but hope it's understandable enough):@docusaurus/theme-live-codeblock
and add it tothemes
.src/theme/CodeBlock.js
and add:This turns into a blank screen with an error message
Uncaught (in promise) ReferenceError: Cannot access '__WEBPACK_DEFAULT_EXPORT__' before initialization
. The reason is that@theme-original/CodeBlock
points to the live codeblock, but@theme-init/CodeBlock
used in the live codeblock points to the user-swizzled one, causing a circular import.I have a feeling that even the maintainers do not have a good understanding of what these aliases do, so I refactored the code a little and added some docs to clarify the internal details of aliasing.
This PR does not solve #4530 though, because Webpack aliases need to deterministically point to one file, so
@theme-init
has to point to the bottommost component. All plugins that try to enhance the same component would still receive the same, unenhanced component when importing from@theme-init
. We can't "relay" the component from one theme to another.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
In the Webpack alias test, I added another plugin to mimic my example above. This time
@theme-init
points to the first plugin,@theme-original
points to the second, and@theme
points to the user-swizzled one.