-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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(core): allow sourcing from multiple static directories #4095
feat(core): allow sourcing from multiple static directories #4095
Conversation
[V1] Deploy preview failure Built without sensitive environment variables with commit b4423b0 https://app.netlify.com/sites/docusaurus-1/deploys/600d08435d5ff20009fbd5ed |
Size Change: 0 B Total Size: 28.8 kB ℹ️ View Unchanged
|
✔️ [V2] 🔨 Explore the source changes: 4901a12 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/6196526d6bc8d9000782b008 😎 Browse the preview: https://deploy-preview-4095--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4095--docusaurus-2.netlify.app/ |
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 for working on this!
What I suggest:
- use
default()
in config validation - get rid of the
STATIC_DIR_NAME
constant, to ensure the configured array is used everywhere - support the static dir array in the mdx loader
This comment has been minimized.
This comment has been minimized.
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 looks nice 👍
We should support static dir array in mdx loader too
As you are on it, one idea I had is to have a i18n/<locale>/static
folder as a convention for localized assets, as we don't have a good way to localize static assets currently
@@ -187,17 +186,17 @@ export default async function start( | |||
// Reduce log verbosity, see https://github.com/facebook/docusaurus/pull/5420#issuecomment-906613105 | |||
stats: 'summary', | |||
}, | |||
static: { | |||
static: siteConfig.staticDirectories.map((dir) => ({ |
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.
oh, hopefully they allow an array here 😅
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.
Yeah, they do:
https://webpack.js.org/configuration/dev-server/#directory
https://webpack.js.org/plugins/copy-webpack-plugin/#root
The one thing I'm slightly worried about though is path collisions. I don't know if any warning should be given in that case
Yes, realized that
I'd always used One thing I didn't particularly like about the MDX loader is it converts static paths to require calls. Before I delve into the internals, as a user, I always thought that I was referencing a URL path with |
Oh yes didn't think about that 😅 this is probably enough for now. Assets may not need to be uploaded to Crowdin. We'll see if someone ask for this extra i18n static path
This is important so that assets enter the Webpack pipeline instead of just being referenced strings. This leads to assets being hashed, providing better long-term caching. And also we might have a better image infra someday, allowing you to use ideal image on markdown files with simple md syntax. Some image-heavy docs have annoying image-related layout shifts atm (can probably be solved with width/height in the short term anyway) |
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 looks great, and it's nice to have moved the dogfood assets!
Just minor changes:
- Support absolute path? I can see some users that may want to do that (those using home-made deployment on their own corporate servers in particular)
- Tests should rather cover arrays with multiple static dirs
This is a WIP