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: add linkTags API #8077

Closed
wants to merge 0 commits into from
Closed

feat: add linkTags API #8077

wants to merge 0 commits into from

Conversation

johnnyreilly
Copy link
Contributor

@johnnyreilly johnnyreilly commented Sep 9, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

I'm trying to add a custom link tag to the header of all the pages on my Docusaurus site. In my case I'm looking to render a web monetization link which looks like this:

<link rel="monetization" content="https://ilp.uphold.com/LwQQhXdpwxeJ" />

It looks like there isn't a way to generically supply global link tags with Docusaurus. At least... until now!

I've implemented a new API called linkTags which is a generic mechanism for rendering link tags. It is modelled after the metadata API and sits alongside it in the ThemeConfig of docusaurus.config.js

Test Plan

I'm not sure where unit tests for this would go - but if you give me a pointer I'll write one! In the meantime you can see a screenshot of this working in this PR and see it working in the preview website also.

Test links

image

Look at that! It works!

Deploy preview: https://deploy-preview-8077--docusaurus-2.netlify.app/

Crack open the devtools and you can see the link tag present and correct.

Related issues/PRs

#8049

@netlify
Copy link

netlify bot commented Sep 9, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit c6f5cf9
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/63372f0de5e2dc0009acea0c
😎 Deploy Preview https://deploy-preview-8077--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented Sep 9, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 56 🟢 98 🟢 100 🟢 100 🟠 80 Report
/docs/installation 🟠 78 🟢 100 🟢 100 🟢 100 🟢 90 Report

@Josh-Cena
Copy link
Collaborator

I don't have any opinions on this, but I wonder whether we should make it a theme API (like metadata) or a core API (like stylesheets). I don't really know why we even have the latter.

@johnnyreilly
Copy link
Contributor Author

johnnyreilly commented Sep 9, 2022

I put it where it is as it seems like the logical bedfellow to metadata. It would feel perhaps a little strange if you globally configured meta tags in one place and link tags in another. This way at least it's consistent. But I don't feel super strongly

@Josh-Cena
Copy link
Collaborator

Me neither. Feels weird since stylesheets does quite the same thing, but I can see why it lives in core (we might do some other optimizations with them).

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label Sep 9, 2022
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.

Looks almost good! Could you add some docs in https://docusaurus.io/docs/api/themes/configuration and possibly https://docusaurus.io/docs/seo?

@johnnyreilly
Copy link
Contributor Author

Awesome - will do! Just heading to bed now; will put some docs up tomorrow I expect.

@johnnyreilly johnnyreilly requested review from Josh-Cena and removed request for slorber and lex111 September 10, 2022 11:21
@johnnyreilly
Copy link
Contributor Author

Do these docs work for you @Josh-Cena?

@johnnyreilly
Copy link
Contributor Author

Paging @Josh-Cena 😅

@Josh-Cena
Copy link
Collaborator

Sorry! All public API additions need to be reviewed by @slorber but he'a on holiday til end of month, so this won't be merged in a while anyway... I would take a look at the documentation later.

@johnnyreilly
Copy link
Contributor Author

Awesome - thanks for the update!

website/docs/seo.md Outdated Show resolved Hide resolved
@johnnyreilly
Copy link
Contributor Author

There's a failing Windows test, but by the looks of it, it's a flaky test and not linked to this PR. My guess is that if you rerun it'll pass.

@slorber
Copy link
Collaborator

slorber commented Sep 28, 2022

What prevents you from using a plugin instead?

The API is not easy to discover but I think this impl should be good enough?

function pluginMonetization(context, options) {
  return {
    name: "plugin-monetization",
    injectHtmlTags() {
      return {
        headTags: [
          {
            tagName: "link",
            attributes: {
              rel: "monetization",
              content: options.monetizationContent
            }
          }
        ]
      };
    }
  };
}

Similarly it should be possible to create a more generic links plugin


Although I think we discussed in the past it might be convenient to add a such shortcut to add global site links more conveniently, I'm not sure it's the responsibility of the theme to do so, as it would be duplicated in another theme that we'd want to implement.

Maybe it's better to add this to createBootstrapPlugin() that is always added to any Docusaurus site in core (used to provide similar shortcuts?

export function createBootstrapPlugin({
  siteDir,
  siteConfig,
}: LoadContext): LoadedPlugin {
  const {
    stylesheets,
    scripts,
    clientModules: siteConfigClientModules,
  } = siteConfig;
  return {
    name: 'docusaurus-bootstrap-plugin',
    content: null,
    options: {
      id: 'default',
    },
    version: {type: 'synthetic'},
    path: siteDir,
    getClientModules() {
      return siteConfigClientModules;
    },
    injectHtmlTags: () => {
      const stylesheetsTags = stylesheets.map(
        (source): string | HtmlTagObject =>
          typeof source === 'string'
            ? `<link rel="stylesheet" href="${source}">`
            : {
                tagName: 'link',
                attributes: {
                  rel: 'stylesheet',
                  ...source,
                },
              },
      );
      const scriptsTags = scripts.map((source): string | HtmlTagObject =>
        typeof source === 'string'
          ? `<script src="${source}"></script>`
          : {
              tagName: 'script',
              attributes: {
                ...source,
              },
            },
      );
      return {
        headTags: [...stylesheetsTags, ...scriptsTags],
      };
    },
  };
}

Also not a fan of "linkTags" naming. We already have stylesheets, scripts and metadata (which afaik is plural of metadatum) so "links" makes more sense to me (also similar to Remix: https://remix.run/docs/en/v1/api/conventions#links)


Side note but stylesheets + scripts are in core/siteConfig, and metadata is in the theme/themeConfig (as you noted), that's a bit weird

https://docusaurus.io/docs/api/themes/configuration#metadata

Maybe we'll want to add a site.metadata in core too for consistency? 🤷‍♂️ Maybe we'll have both site.metadata + theme.metadata? 🤷‍♂️

Some important thing to consider: we set some global non-core opinionated meta in the theme currently (I think just <meta name="twitter:card" content="summary_large_image" />) and users should be able to easily override this on a global site level. Declaration order of metadata will matter to keep this behavior.

Unlike metadata, I'm not sure links are supposed to override each others (any use-case for this?).
That's worth checking if React Helmet does override/deduplicate itself links (it does for meta).

For example, let's imagine we want to fund Docusauurs with your monetization system, and added our own monetization link by default to all newly created sites: could users override such link? 🤷‍♂️

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2022

What about just using the synthetic boottrap plugin above to provide a siteConfig.head or siteConfig.headTags API?

Like suggested here: #8049 (comment)

In the end it's the most flexible way to add any tag in the head, no matter what it is. anything else would just be a shortcut.

I'd rather start with more generic, less convenient apis first, and then add shortcuts like linkTags later (eventually). That the philosophy of the extensible web manifesto (https://extensiblewebmanifesto.org/).

Note that we already have a low-level api (plugin), but agree this shortcut (siteConfig.head) would be convenient.

@johnnyreilly
Copy link
Contributor Author

The API is not easy to discover but I think this impl should be good enough?

It works - but this is hard to discover and very verbose for what you're trying to achieve (add some link tags). Feels like you have to buy a car when you only want to drive a mile 😄

Also not a fan of "linkTags" naming. We already have stylesheets, scripts and metadata (which afaik is plural of metadatum) so "links" makes more sense to me (also similar to Remix: https://remix.run/docs/en/v1/api/conventions#links)

Happy with links as a name; would be happy to switch.

What about just using the synthetic boottrap plugin above to provide a siteConfig.head or siteConfig.headTags API?

Like suggested here: #8049 (comment)

I'd be happy to have a go at this - do you have any examples in the codebase of how this is done already? I can't see any piping examples in the codebase so far?

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2022

Similarly, it can also be handled in the core synthetic bootstrap plugin:

export function createBootstrapPlugin({
  siteDir,
  siteConfig,
}: LoadContext): LoadedPlugin {
  const {
    stylesheets,
    scripts,
    head, // <==== NEW
    clientModules: siteConfigClientModules,
  } = siteConfig;
  return {
    name: 'docusaurus-bootstrap-plugin',
    // other things
    injectHtmlTags: () => {
      const stylesheetsTags = [];
      const scriptsTags = [];
      return {
        headTags: [
          ...head, // <==== NEW
          ...stylesheetsTags, ...scriptsTags],
      };
    },
  };
}

Does it make sense?

@johnnyreilly
Copy link
Contributor Author

Sorry no - I don't follow! I don't know what the createBootstrapPlugin does.... And I'm not sure what consumption would then look like in a docusaurus.config.js?

@johnnyreilly
Copy link
Contributor Author

johnnyreilly commented Sep 29, 2022

I think what you're saying is add something extra to this:

export function createBootstrapPlugin({

But I don't know how that connects to the docusaurus.config.js?

I guess the question is: how do users use it? I'm guessing that all users are somehow using the createBootstrapPlugin and they just don't know it? Is that right? If so, how does that play out? Where would they supply links? Or rather head? And what does that look like?

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2022

I'm guessing that all users are somehow using the createBootstrapPlugin and they just don't know it?

Exactly: all sites implicitly have this plugin by default.

I don't know what the createBootstrapPlugin does

It's just a default plugin added to all docusaurus sites. We use it to provide shortcuts because some features are already provided by lifecycle plugins so we can just implement something in core with... a plugin.

If we were to provide an siteConfig.webpack.configure() api, we'd use this too ;)


Use the code above where I added the NEW comments, and users will be able to use in siteconfig:

{head: {tag: "link", attributes: {rel: "monetization"}}}

@johnnyreilly
Copy link
Contributor Author

okay cool and they'd do it at the same level as stylesheets by the looks of it?

I think I could have a crack at this. Probably at the weekend.

@slorber
Copy link
Collaborator

slorber commented Sep 29, 2022

Yes, I think handling siteConfig.stylesheets, siteConfig.scripts and siteConfig.head in the same way makes sense. The 2 former are just shortcuts for the later which is more low-level and flexible.

I'm not against adding a siteConfig.links too as it looks like a convenient shortcut and if we already have 2 similar shortcuts, so why not 3?

But the more important is to provide the low-level API so that if someone wants to add a weird unexpected tag to head (not script, link nor stylesheet), they could. For some reason we started by providing shortcuts instead of the more flexible low-level API 🤷‍♂️

@johnnyreilly
Copy link
Contributor Author

Cool - I think I get it. I'll have a go and report back!

@johnnyreilly
Copy link
Contributor Author

johnnyreilly commented Sep 30, 2022

Closed in favour of @slorber's suggested alternate implementation: #8151

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.

None yet

5 participants