Allow opting out of `favicon` injection

jsamr commented 3 years ago

🚀 Feature

Currently, the favicon field is required, and consumers can't opt-out to implement custom favicon logic via swizzling or plugins.

Have you read the Contributing Guidelines on issues?

Yes.

Motivation

I'm writing a plugin to autogenerate different head tags related to favicon and theming via realfavicongenerator API. It works great, but I am stuck with a duplicated favicon.

Pitch

The favicon config field could stay mandatory, but with an option to explicitly opt-out via setting false as a value.

slorber commented 3 years ago

👍 easy issue, if anyone wants to contribute please do

satya-nutella commented 3 years ago

@slorber I would like to take up this issue. I am a new contributor here. Any directions would be awesome

jsamr commented 3 years ago

@meehawk I had done a little research and took notes so I'm going to share it here:

For each theme, you need to handle the case where favicon is false. See

Favicon is also used in the blog plugin:

https://github.com/facebook/docusaurus/blob/f71e83450ff21476d1ffb0717aa1a26f9949b221/packages/docusaurus-plugin-content-blog/src/blogUtils.ts#L89

Then, you need to change the Joi validator and the typescript type to allow false:

Finally of course, add tests covering the false case.

@slorber please correct me if I missed something. Perhaps it would be best to implement a Favicon component ; there is a lot of duplicated logic across themes. But IDK, @slorber, if you have plans for a common ui package.

slorber commented 3 years ago

It is not necessary to modify the bootstrap theme. It is currently unmaintained and we'll figure out how to refactor our code to support multiple themes consistently without duplicating code.

@jsamr the blog plugin uses the global site favicon for its rss/atom feeds. What kind of behavior do you expect for those feeds? Using config.favicon = false means RSS feeds won't have favicons, so maybe we should allow the blog plugin feeds to provide a custom favicon?

slorber commented 3 years ago

@meehawk thanks for your help, I think @jsamr gave decent information to get started, let me know if you have any concrete question

jsamr commented 3 years ago

@jsamr the blog plugin uses the global site favicon for its rss/atom feeds. What kind of behavior do you expect for those feeds? Using config.favicon = false means RSS feeds won't have favicons, so maybe we should allow the blog plugin feeds to provide a custom favicon?

The plugin I have implemented uses injectHtmlTags lifecycle API; I wonder if the logic of using a custom favicon could be automated and handled by the docusaurus core? If a <link rel="icon" ... is found, then don't use default favicon and set the global favicon to this element href. It is a bit more complicated since different resolution might co-exist, hence multiple match. But a great advantage is that the blog plugin could use this information with no need for extra configuration. A pitfall could be the plugin loading order, as I don't really know the internals and limitations (are plugins loaded synchronously?) @slorber What do you think?

satya-nutella commented 3 years ago

@slober @jsamr thank you so much for all the info. I will try to work on this Monday on

slorber commented 3 years ago

The plugin I have implemented uses injectHtmlTags lifecycle API; I wonder if the logic of using a custom favicon could be automated and handled by the docusaurus core?

If you use the Head component from Docusaurus / React Helmet (https://github.com/nfl/react-helmet), adding a 2nd favicon is likely to override the default one. If you inject the favicon in any other way, Helmet probably can't do the deduplication itself. Have you tried implementing your custom logic with <Head> instead?

If a <link rel="icon" ... is found, then don't use default favicon and set the global favicon to this element href. It is a bit more complicated since different resolution might co-exist, hence multiple match.

The static HTML we output must be rendered through nodejs and contain your final favicon. Apart the React tree used for server-rendering, we don't evaluate other scripts that you add to that page at build time and can't detect that it will add an extra favicon to the DOM. Your static HTML really should contain the final favicon after a build, I don't think adding the favicon later with client-side JS is a good idea/practice but not sure.

But a great advantage is that the blog plugin could use this information with no need for extra configuration.

The blog plugin has to compute a favicon URL at buid time, and the only way to do that is by reading your configuration file. It really shouldn't read your static HTML files. And considering you might hack docususaurus so that / homepage use favicon1 and /docs/myDoc use favicon2, which favicon should appear in the blog RSS feed in the end?

I don't really understand your usecase @jsamr: what are the pages of your site, and which favicon each page should have exactly? Couldn't we add a SiteFavicon component that you would easily swizzle and customize according to the current path or something?

jsamr commented 3 years ago

I don't think adding the favicon later with client-side JS is a good idea/practice but not sure.

@slorber As stated before, I use injectHtmlTags function from the plugin module:

module.exports = function rfgPlugin() {
  return {
    // Other lifecycle methods
    // ...
    injectHtmlTags() {
      return {
        // These tags are generated with RealFaviconGenerator API
        headTags: [
          {
            tagName: "link",
            attributes: {
              rel: "icon",
              type: "image/png",
              sizes: "32x32",
              href: "/favicons/favicon-32x32.png",
            },
          },
          {
            tagName: "link",
            rel: "manifest",
            href: "/favicons/site.webmanifest",
          },
          // ... other tags
        ],
      };
    },
  };
};

The plugin generates assets in the static folder. I don't know exactly how and when headTags are consumed by docusaurus core, but my understanding is that it should be at build time, with nodejs. My suggestion was that docusaurus core should be able to identify when a favicon is registered that way. Perhaps the tag object could contain a new property, type: 'favicon', for explicit overriding...

slorber commented 3 years ago

I'll need to see your plugin to understand what you are trying to do @jsamr

Here you show a static favicon string, why not using favicon: "/favicons/favicon-32x32.png" in the config directly instead of doing that in a plugin?

Is your favicon supposed to be constant for your whole site?

yes: I'm unable to understand why config is not good enough then
no: then how Docusauurs can know which favicon to pick for the blog rss feed for example?

What is the relationship with the real favicon API and where/how/when is it called? What are the inputs/outputs

jsamr commented 3 years ago

@slorber First of all, thank you for your patience and sorry if I wasn't clear enough. The plugin is here (beware, code is in a draft state) https://github.com/meliorence/react-native-render-html/blob/b2431d23668ca2dccd2389c8435401ed1ccc68c6/doc-tools/doc-docsusaurus-rfg-plugin/src/index.ts

Here you show a static favicon string, why not using favicon: "/favicons/favicon-32x32.png" in the config directly instead of doing that in a plugin?

Well, no, "These tags are generated with RealFaviconGenerator API"... Something like this:

function generateHeadTags(apiResponseData): HtmlTags[] {
  const html = apiResponseData.html; // The head tags in HTML
  const dom = parseDOM(html);
  return dom.map((n) => {
    const element = n as Element;
    return {
      tagName: element.tagName,
      attributes: element.attribs,
    };
  });
}

Moreover, this plugin doesn't insert one <link>, but many tags, see https://realfavicongenerator.net/ Finally, I thought it would be nice to publish a plugin to share with the community.

The "input" of this API is a big config object with options for each target (Windows 10, iOS, Chrome on Android, Safari ...), see the design entry of this object literal:

https://github.com/meliorence/react-native-render-html/blob/b2431d23668ca2dccd2389c8435401ed1ccc68c6/doc-tools/doc-docsusaurus-rfg-plugin/src/index.ts#L67

The body of its response contains the link to an archive file with all the generated assets and an html field with all the tags to insert in the head.

Moreover, the plugin will produce two categories of files:

Static assets, example here: https://github.com/meliorence/react-native-render-html/tree/master/apps/website/static/favicons
A JSON file containing the cached headTags generated from the API (goes somewhere in .docusaurus folder)

slorber commented 3 years ago

Moreover, this plugin doesn't insert one , but many tags

Not sure to understand why it needs multiple links? Can you explain?

As a workaround, you could write a json file as a pre-build step (with the cli), and then use the "main" favicon as docusaurus config, and only insert the remaining tags? (with the "main favicon" filtered, as it's already inserted through config).

For sure your plugin would need an extra manual step, asking the user to configure site favicon to the path your plugin writes to, but that seems ok to me.

Not really related but:

should we make injectHtmlTags receive plugin loaded content?
can you add your site to the showcase with opensource and design tags?
we are planning to redesign admonitions in Infima, maybe you'd be interested to provide feedback: https://github.com/facebookincubator/infima/issues/55

satya-nutella commented 3 years ago

@slorber @jsamr finding myself a bit lost here. Apologies since I am new here

slorber commented 3 years ago

@meehawk we are trying to figure out what we should do, so for now nothing to implement :)

jsamr commented 3 years ago

@slorber

Not sure to understand why it needs multiple links? Can you explain?

Below is an excerpt of the HTML markup returned by RFG API:

<link rel="apple-touch-icon" sizes="180x180" href="/react-native-render-html/favicons/apple-touch-icon.png">
<link rel="icon" type="image/png" sizes="32x32" href="/react-native-render-html/favicons/favicon-32x32.png">
<link rel="icon" type="image/png" sizes="16x16" href="/react-native-render-html/favicons/favicon-16x16.png">
<link rel="manifest" href="/react-native-render-html/favicons/site.webmanifest">
<link rel="mask-icon" href="/react-native-render-html/favicons/safari-pinned-tab.svg" color="#8181ff">
<link rel="shortcut icon" href="/react-native-render-html/favicons/favicon.ico">
<meta name="msapplication-TileColor" content="#000040">
<meta name="msapplication-config" content="/react-native-render-html/favicons/browserconfig.xml">
<meta name="theme-color" content="#8181ff">

hence the numerous <link>!

As a workaround, you could write a json file as a pre-build step (with the cli), and then use the "main" favicon as docusaurus config, and only insert the remaining tags? (with the "main favicon" filtered, as it's already inserted through config).

For sure your plugin would need an extra manual step, asking the user to configure site favicon to the path your plugin writes to, but that seems ok to me.

True, but we would still end-up with a duplicated favicon ... Not sure what's the best solution here! But an option to disable favicon generation, and supply a favicon manually for the blog plugin seems reasonable to me.

should we make injectHtmlTags receive plugin loaded content?

Yes :pray:, that would make the implementation cleaner...

can you add your site to the showcase with opensource and design tags?

Done #5022

slorber commented 3 years ago

True, but we would still end-up with a duplicated favicon ... Not sure what's the best solution here! But an option to disable favicon generation, and supply a favicon manually for the blog plugin seems reasonable to me.

I don't see why. You can pass favicon-32x32.png as the main site favicon in config (and will be used in blog plugin), and you can add the remaining favicons through the plugin. You can filter favicon-32x32.png in the plugin so that it's not inserted twice.

Anyway, using the favicon in feeds does not look so important. In practice, it's only used in the Atom feed, and there's no icon in the RSS spec. We can generate decent feeds without it.

Currently, we have favicon: Joi.string().required(),, we should just remove the required() here and handle the cases where this is undefined/not provided + update doc (https://docusaurus.io/docs/docusaurus.config.js#favicon). Do you want to do that @meehawk ?

I'll make sure injectHtmlTags receive the loaded content (https://github.com/facebook/docusaurus/pull/5037)

jsamr commented 3 years ago

I don't see why. You can pass favicon-32x32.png as the main site favicon in config (and will be used in blog plugin), and you can add the remaining favicons through the plugin. You can filter favicon-32x32.png in the plugin so that it's not inserted twice.

Right, although we would miss some attributes such as size which I guess can help browsers pick up the best resolution ... However, for those cases where the original "pristine" favicon is an svg, I could direct users to set the svg source as the default favicon (in docusaurus config), and RFG-generated links would act as safe fallbacks.

slorber commented 3 years ago

Allowing undefined favicon here: https://github.com/facebook/docusaurus/pull/5054

facebook / docusaurus