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(v2): injectHtmlTags API to inject head and/or body html tags #2057

Merged
merged 11 commits into from
Nov 30, 2019

Conversation

endiliey
Copy link
Contributor

@endiliey endiliey commented Nov 27, 2019

Motivation

This should partially address #1995 and #2000

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

  • Newly added test passes

test html tags

  • Using example from the documentation I added

Dev
inject body tags dev
inject head tags dev

Prod
inject body tags prod
inject head tags prod

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Nov 27, 2019
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 27, 2019

Deploy preview for docusaurus-2 ready!

Built with commit 3559a11

https://deploy-preview-2057--docusaurus-2.netlify.com

@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Nov 27, 2019

Deploy preview for docusaurus-preview ready!

Built with commit 3559a11

https://deploy-preview-2057--docusaurus-preview.netlify.com

@endiliey
Copy link
Contributor Author

endiliey commented Nov 27, 2019

With this, I was able to solve the noflash problem but the toggle didnt change until the next react render :O (suspecting some hydration bug/ react-toggle shallow props comparison)
aa

Edit: used devtool, passed props and state is correct. But toggle not displaying correctly :x
image

Anyway, if someone want the source code to fix no flash

const path = require('path');

const noFlash = `(function() {
  // Change these if you use something different in your hook.
  var storageKey = 'theme';

  function setDataThemeAttribute(theme) {
    document.querySelector('html').setAttribute('data-theme', theme);
  }
  
  var preferDarkQuery = '(prefers-color-scheme: dark)';
  var mql = window.matchMedia(preferDarkQuery);
  var supportsColorSchemeQuery = mql.media === preferDarkQuery;
  var localStorageTheme = null;
  try {
    localStorageTheme = localStorage.getItem(storageKey);
  } catch (err) {}
  var localStorageExists = localStorageTheme !== null;

  // Determine the source of truth
  if (localStorageExists) {
    // source of truth from localStorage
    setDataThemeAttribute(localStorageTheme);
  } else if (supportsColorSchemeQuery) {
    // source of truth from system
    var theme = mql.matches ? 'dark' : '';
    setDataThemeAttribute(theme);
    localStorage.setItem(storageKey, theme);
  } else {
    // source of truth from document
    var theme = document.querySelector('html').getAttribute('data-theme');
    localStorage.setItem(storageKey, theme);
  }
})();`

module.exports = function(context, options) {
  const {customCss} = options || {};
  return {
    name: 'docusaurus-theme-classic',

    getThemePath() {
      return path.resolve(__dirname, './theme');
    },

    getClientModules() {
      return ['infima/dist/css/default/default.css', customCss];
    },

    injectHtmlTags() {
      return {
        preBodyTags: [
          {
            tagName: 'script',
            attributes: {
              type: 'text/javascript',
            },
            innerHTML: noFlash
          },
        ],
      }
    }
  };
};

@endiliey endiliey added the pr: new feature This PR adds a new API or behavior. label Nov 27, 2019
@endiliey
Copy link
Contributor Author

endiliey commented Nov 27, 2019

cc @lex111 in case later on you wanted to fix the no flash problem

Edit: we can also continue #2000 adding rss feed to head

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

I think we need to think about the API and approach for a bit more before committing. Have you seen https://www.gatsbyjs.org/docs/ssr-apis/? We might be able to get some inspiration from it.

@endiliey
Copy link
Contributor Author

Ready for re-review @yangshun

dev
prebody postbody

prod
prebody postbody prod

Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Good job! 👏

@endiliey
Copy link
Contributor Author

@yangshun, safe to merge ? 🤣

do you want to fix the dark mode no flash with this ? @lex111

@yangshun
Copy link
Contributor

Gimme another day to review it. One question - is there a way to specify <script> contents via using the object syntax?

@endiliey
Copy link
Contributor Author

endiliey commented Nov 29, 2019

@yangshun sure no worries & no rush. Was just making sure in case I misunderstood your comment previously. We still have #2048 to address to before alpha.37 release. Lot of commits has been added since alpha.36.

One question - is there a way to specify <script> contents via using the object syntax?

Yes. It's called innerHTML.

Edit: I also allow string style way

See #2057 (comment) as example on how i injected noflash

It's based on html-webpack-plugin HtmlTagObject API so I believe it's very modular & flexible

@endiliey
Copy link
Contributor Author

<div>test</div>

is just

{
  tagName: 'div',
  innerHTML: 'test'
}

It's like a HTML5 Spec. That's why React has

<div dangerouslySetInnerHTML={'test'} />;

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

do you want to fix the dark mode no flash with this ?

I'll try it!

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

With this, I was able to solve the noflash problem but the toggle didnt change until the next react render :O (suspecting some hydration bug/ react-toggle shallow props comparison)

@endiliey it's strange, I use your code for noflash, but I don’t see any bugs, I mean, everything works fine! Can you please describe in steps how to reproduce some hydration bug with toggle?

@endiliey
Copy link
Contributor Author

endiliey commented Nov 29, 2019

@lex111 is it ? Maybe css caching or something. Did you make sure you use production build ?
My step is just

  1. Build production
  2. Serve production file with https://www.npmjs.com/package/serve
  3. Toggle dark mode, refresh

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

@endiliey Oooh, now I see that! But why is this issue not in dev mode?

@endiliey
Copy link
Contributor Author

@lex111 because dev is just a SPA. Production is staticly rendered HTML (works without JavaScript), and when the JS loads, it hydrates the DOM.

Did you try with chrome dev tool ? It's getting correct props.

Like if I changed the toggle component to pure text, it's working well. I wonder if its react-toggle bug or something..

@endiliey
Copy link
Contributor Author

Aha, I think I might found the problem.
maybe because https://reactjs.org/docs/react-dom.html#hydrate

Same as render(), but is used to hydrate a container whose HTML contents were rendered by ReactDOMServer. React will attempt to attach event listeners to the existing markup.

React expects that the rendered content is identical between the server and the client. It can patch up differences in text content, but you should treat mismatches as bugs and fix them. In development mode, React warns about mismatches during hydration. There are no guarantees that attribute differences will be patched up in case of mismatches. This is important for performance reasons because in most apps, mismatches are rare, and so validating all markup would be prohibitively expensive.

If you intentionally need to render something different on the server and the client, you can do a two-pass rendering. Components that render something different on the client can read a state variable like this.state.isClient, which you can set to true in componentDidMount(). This way the initial render pass will render the same content as the server, avoiding mismatches, but an additional pass will happen synchronously right after hydration. Note that this approach will make your components slower because they have to render twice, so use it with caution.

Because in HTML it's actually not checked (we don't know if user local storage is 'dark' or non dark) yet. So when its hydrating, the client is mismatching with server but not patched up until next render.

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

That's the point, props are same. I think we need to additionally set the proper prop in the component itself, as is done in Gatsby.

@endiliey
Copy link
Contributor Author

endiliey commented Nov 29, 2019

@lex111

So actually gatsby site (in this case overreacted.io) cheated by not rendering the Toggle in the server. Then after hydration, it will render it. Doing a two pass rendering.

https://github.com/gaearon/overreacted.io/blob/7eee68bd9f20e72f04dbe5422977a8cf35102e35/src/components/Layout.js#L107-L138

(JavaScript disabled)
image

(Throttled)
no toggle on ssr

We need to somehow borrow that idea. Maybe passingPassing disabled=true props to Toggle in server and then when componentdidmount, we pass disabled=false to toggle. This will force render.

I got it working :| Although my code sux lol

good stuff

Simply add this in navbar

// Navbar.js
const [isClient, setIsClient] = useState(false);

useEffect(() => {
    if (!isClient) {
      setIsClient(true);
    }
  }, []);

And pass disabled props to all Toggle component in Navbar

<Toggle disabled={!isClient} />

This is the throttled version.
behind the scene

(JavaScript disabled)
image

Edit from future
Maybe better if isClient is set on Docusaurus context so that there won't be many unnecessary re-render.

// docusaurus/client/App.js
function App() {
  const [isClient, setIsClient] = useState(false);
  const {stylesheets, scripts} = siteConfig;

  useEffect(() => {
    setIsClient(true);
  }, [])
  return (
    <DocusaurusContext.Provider value={{siteConfig, isClient}}>

And toggle can just consume from that.

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

Hmm, it's difficult case, but I see in the gif how the toggle changes, can we avoid this?

@endiliey
Copy link
Contributor Author

@lex111 we can't, unless we don't mind doing it like overreacted io, hiding it.
But if we hide it, due to responsive design it will be something like this. I guess its css related and we need some tweaks. We can try to put some space container there that isn't null.

toggle not changing

Removing the GitHub works well 😆
remove github

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

But if we hide it, due to responsive design it will be something like this. I guess its css related and we need some tweaks. We can try to put some space container there that isn't null.

Cool! In my opinion this is the best option (?). Using CSS is easy to do it.

@endiliey
Copy link
Contributor Author

endiliey commented Nov 29, 2019

@lex111 do you want to work on it ? I've shared my experiment with it, and I think you can do a better job on this. (My front-end sense & css & react level is n00b 😭)

I'm gonna sleep now (its midnight here) .... Thank you for the time btw !!

Edit: in case u want my wip code 28c4623

@lex111
Copy link
Contributor

lex111 commented Nov 29, 2019

@endiliey yeah, I will care about it!

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

See comments. I wonder if there's a way/need to make this feature page-specific? They're global as of now which is ok because it's typically meant for global stuff but just wondering if there will be a need for page-specific tags.

@@ -5,8 +5,13 @@
<meta name="viewport" content="width=device-width">
<meta name="generator" content="Docusaurus">
<title><%= htmlWebpackPlugin.options.title %></title>
<%= htmlWebpackPlugin.options.headTags %>
<%= htmlWebpackPlugin.tags.headTags %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this leftover by mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no its not. So html webpack plugin by default will inject the required script at postbody. So since we want to add our own postbody tags, we have to set “inject:false” to htmlwebpackplugin and then put the injection place. See https://github.com/jantimon/html-webpack-plugin/tree/master/examples/custom-insertion-position

Copy link
Contributor Author

@endiliey endiliey Nov 30, 2019

Choose a reason for hiding this comment

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

*at posthead and postbody.

<div id="__docusaurus"></div>
<%= htmlWebpackPlugin.tags.bodyTags %>
Copy link
Contributor

Choose a reason for hiding this comment

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

This too.

@endiliey
Copy link
Contributor Author

See comments. I wonder if there's a way/need to make this feature page-specific? They're global as of now which is ok because it's typically meant for global stuff but just wondering if there will be a need for page-specific tags.

It needs to be done globally IMO.
For page specific html tags, that needs to be done client side IMO like using helmet for head.

Its because when you go to v2.docusaurus.io, you only load the html once, and the rest of the navigation is using that one HTML and all is just a SPA navigation.
its more of a SPA problem for me.

@endiliey
Copy link
Contributor Author

endiliey commented Nov 30, 2019

To make it feature page specific, then we need something like docusaurus-browser or docusaurus-ssr :)

Which is kinda different from this 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.

6 participants