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: shorthands for themes/plugins/presets configuration #5930

Merged
merged 28 commits into from
Dec 3, 2021

Conversation

fsmaia
Copy link
Contributor

@fsmaia fsmaia commented Nov 11, 2021

Motivation

See #5883

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  1. Run manually, packing with npm pack, and installing in a custom docusaurus.config.js
  2. Post the screenshot here
  3. Include this scenario in the test file (init.test.ts), figuring out a way to mock require.resolve (as it won't be physically installed), or then add a demo plugin in the fixture package.json file)

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

netlify bot commented Nov 11, 2021

✔️ [V2]

🔨 Explore the source changes: ddd3385

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61aa4e160eaabc0007784aa3

😎 Browse the preview: https://deploy-preview-5930--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Nov 11, 2021

⚡️ Lighthouse report for the changes in this PR:

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

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

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 11, 2021

I indexed plugins by type to identify plugin and theme aliases. This affected loadPluginConfigs.

Copy link
Collaborator

@Josh-Cena Josh-Cena 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 cool

A little POC using our own website will help us see if it's working

packages/docusaurus/src/commands/swizzle.ts Outdated Show resolved Hide resolved
@Josh-Cena Josh-Cena changed the title Support shorthand for themes/plugins/presets configuration feat: shorthands for themes/plugins/presets configuration Nov 12, 2021
@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Nov 12, 2021
@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 13, 2021

Thanks, looks cool

A little POC using our own website will help us see if it's working

Actually in the next commit I'll apply this same engine for presets, and I thought about using the examples folder to use the shorthands.

I really care about automated tests, but emulating a Node package resolve is very complex, and I would end up mocking too much and the test would practically be a snapshot over the resolve attempts (asserting mapping a string with some prefixes).

@Josh-Cena
Copy link
Collaborator

Hmm, just replacing all the names in our docusaurus.config.js is fine

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 14, 2021

Hmm, just replacing all the names in our docusaurus.config.js is fine

Do you think that the preset part should be applied in a separate PR?

By that I mean accepting, for instance, classic instead of @docusaurus/preset-classic. Btw it's the exact same code (adding a loadPresetModule and a resolvePresetModule).

@Josh-Cena
Copy link
Collaborator

Do you think that the preset part should be applied in a separate PR?

Yes—I haven't reviewed the impl in detail, but I think it would be trivial to add and weird to not

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 16, 2021

I preferred to add this resolver to presets too, as the title/issue already suggests it, and updated it in the examples folder.

It is still working fine:
localhost_3000_

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 16, 2021

Example when an invalid preset/plugin is provided:
image

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 17, 2021

Just executed with another project and the theme resolution is working now.

@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 19, 2021

@Josh-Cena, could you help me identifying why the E2E didn't work?

@Josh-Cena
Copy link
Collaborator

This is a known problem that I fixed in #5958. Don't worry @fsmaia this will be solved after that one gets merged.

@Josh-Cena
Copy link
Collaborator

Hey @fsmaia FYI your approach didn't work well with swizzling because of how our swizzling script tries to resolve theme names through the config. To simplify things (I may revisit that script in the future, the setup looks convoluted) I refactored your code to resolve the module names directly after loading the plugin config. The diff also looks much cleaner this way.

@Josh-Cena Josh-Cena linked an issue Nov 27, 2021 that may be closed by this pull request
2 tasks
@fsmaia
Copy link
Contributor Author

fsmaia commented Nov 29, 2021

Amazing! Big thanks!

@Josh-Cena Josh-Cena added the status: awaiting review This PR is ready for review, will be merged after maintainers' approval label Dec 2, 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.

thanks, LGTM, just did some little modifications

// eslint-disable-next-line no-restricted-syntax
for (const module of modulePatterns) {
try {
moduleRequire.resolve(module);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok for that.

Throwing exceptions has a runtime cost, but it's probably not very important here.
I don't know if there's a fail-safe way to resolve a module in nodejs but if there is one, that would be preferable.

@@ -24,7 +24,7 @@ const dogfoodingPluginInstances = [
],

[
'@docusaurus/plugin-content-blog',
Copy link
Collaborator

Choose a reason for hiding this comment

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

going to dogfood other forms of plugin names here, it's the purpose of dogfooding to have a variety of settings ;)

| --- | --- |
| `awesome` | `docusaurus-plugin-awesome` |
| `sitemap` | [`@docusaurus/plugin-sitemap`](./api/plugins/plugin-sitemap.md) |
| `@joshcena` | `@joshcena/docusaurus-plugin` (the only possible resolution!) |
Copy link
Collaborator

Choose a reason for hiding this comment

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

replacing with @mycompany which is more likely to happen and less confusing for docs readers

@@ -66,15 +125,15 @@ module.exports = {
// ...
plugins: [
[
'@docusaurus/plugin-content-blog',
Copy link
Collaborator

Choose a reason for hiding this comment

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

just thinking out loud but maybe we'll want in a future to just be able to use "blog"?

The "content-blog" thing is a bit weird for the users IMHO

Copy link
Collaborator

Choose a reason for hiding this comment

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

Quite similarly I'm not fan of the complexity of our i18n paths, I wish we could just have i18n/fr/blog

).toThrowErrorMatchingInlineSnapshot(`
"Docusaurus was unable to resolve the \\"@docusaurus/plugin-content-doc\\" plugin. Make sure one of the following packages are installed:
- @docusaurus/plugin-content-doc
- @docusaurus/docusaurus-plugin-plugin-content-doc"
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: added this test, but this resolve name looks a bit weird.

I'll still merge, but if you feel we should do something about it 🤷‍♂️ feel free to open another PR

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.

Support shorthand for themes/plugins/presets configuration
5 participants