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(core): allow sourcing from multiple static directories #4095

Merged
merged 16 commits into from
Nov 18, 2021

Conversation

oriooctopus
Copy link
Contributor

This is a WIP

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jan 24, 2021
@netlify
Copy link

netlify bot commented Jan 24, 2021

@github-actions
Copy link

Size Change: 0 B

Total Size: 28.8 kB

ℹ️ View Unchanged
Filename Size Change
website/build/blog/2017/12/14/introducing-docusaurus/index.html 21.7 kB 0 B
website/build/docs/introduction/index.html 180 B 0 B
website/build/index.html 6.88 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Jan 24, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 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

@github-actions
Copy link

github-actions bot commented Jan 24, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 92
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4095--docusaurus-2.netlify.app/

@oriooctopus oriooctopus marked this pull request as draft January 24, 2021 17:49
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 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

packages/docusaurus/src/commands/build.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/commands/build.ts Show resolved Hide resolved
packages/docusaurus/src/commands/start.ts Outdated Show resolved Hide resolved
packages/docusaurus/src/server/configValidation.ts Outdated Show resolved Hide resolved
@HonkingGoose

This comment has been minimized.

@slorber slorber changed the title [WIP] Implementaion of multiple directory static sourcing [WIP] Implementation of multiple directory static sourcing Feb 17, 2021
@Josh-Cena Josh-Cena marked this pull request as ready for review November 15, 2021 13:57
@Josh-Cena Josh-Cena self-requested a review as a code owner November 15, 2021 13:57
@Josh-Cena Josh-Cena changed the title [WIP] Implementation of multiple directory static sourcing feat(core): allow sourcing from multiple static directories Nov 17, 2021
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Nov 17, 2021
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.

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) => ({
Copy link
Collaborator

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 😅

Copy link
Collaborator

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

@Josh-Cena
Copy link
Collaborator

We should support static dir array in mdx loader too

Yes, realized that

one idea I had is to have a i18n/<locale>/static folder as a convention for localized assets

I'd always used /static/zh-Hans/img/ instead, which also works great with URL mapping. But seems i18n/zh-Hans/static works better with Crowdin🤔

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 /img/docusaurus.png instead of a file path ㄟ(▔,▔)ㄏ

@slorber
Copy link
Collaborator

slorber commented Nov 17, 2021

I'd always used /static/zh-Hans/img/ instead, which also works great with URL mapping. But seems i18n/zh-Hans/static works better with Crowdin🤔

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

One thing I didn't particularly like about the MDX loader is it converts static paths to require calls.

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)

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Nov 18, 2021

@slorber I'm done 👍 Things are working very well and we also get to separate the dogfooding assets from the actual assets the site is using. Also separated Markdown assets fully from static assets documentation—after this is merged we can close #3198 as well

@Josh-Cena Josh-Cena linked an issue Nov 18, 2021 that may be closed by this pull request
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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Static assets doc on 2 pages
5 participants