feat(v2): baseUrl config issues: show help message if css/js can't load #3621

jcs98 · 2020-10-21T13:24:51Z

Motivation

Aims to implement #3584

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

WIP

docusaurus-bot · 2020-10-21T13:31:24Z

Deploy preview for docusaurus-2 ready!

Built with commit e053743

https://deploy-preview-3621--docusaurus-2.netlify.app

slorber

see comments

slorber · 2020-10-22T12:52:35Z

packages/docusaurus/src/client/App.tsx

@@ -27,6 +41,7 @@ function App(): JSX.Element {
  return (
    <DocusaurusContext.Provider
      value={{siteConfig, siteMetadata, globalData, isClient}}>
+      <CSSLoadingWarningMessage isClient />


Suggested change

<CSSLoadingWarningMessage isClient />

<CSSLoadingWarningMessage isClient={isClient} />

Hi,
The pre-commit hook changed this from isClient={isClient} to isClient

slorber · 2020-10-22T12:59:41Z

packages/docusaurus/src/client/App.tsx

+        You site CSS did not load properly. Your baseUrl setting is probably
+        bad.{' '}
+      </span>
+      {isClient && <span>Maybe try baseUrl = {location.pathname}</span>}


Here it's not totally correct.

If we are on:

/myAppBaseUrl/, we should recommend the user to try baseUrl: '/myAppBaseUrl/'

/myAppBaseUrl/docs/myDoc, we should recommend the user to try baseUrl: '/myAppBaseUrl/'

What you currently have is, if you are on:

/myAppBaseUrl/, you recommend the user to try baseUrl: '/myAppBaseUrl/'

/myAppBaseUrl/docs/myDoc, you recommend the user to try baseUrl: '/myAppBaseUrl/docs/myDoc' => not a good recommendation

Not sure what's the best way to implement this, but my initial suggestion was rather to only display this message on the homepage, to avoid increasing all the site's page size with this additional, warning, and as it's the URL users will tend to browse first when trying their new deployment.

In any case we should only recommend valid baseUrl settings to the user, or he'll be more confused than ever.

So should I remove the suggestion for now?

jcs98 · 2020-10-22T17:08:24Z

I've made my first attempt at detecting if the CSS has not loaded. As per the description in the issue, I hide the message on the first remotely-loaded CSS file.
It also seems to work for the 2 conditions mentioned in the issue's description.

http://localhost:5000/build/ after yarn serve website

http://localhost:3000/ after yarn start

slorber

Nice try, but you can see for yourself in the deploy preview that it does not work 🤪 :

https://deploy-preview-3621--docusaurus-2.netlify.app/classic/

For sites with correct baseUrl config, this warning should normally have no visible effect, it should only affect sites with bad baseUrl configs.

Read my suggestions here to understand better what should be done: #3584 (comment)

slorber · 2020-10-23T14:53:02Z

packages/docusaurus/src/client/App.tsx

+    } catch (e) {}
+  }
+  return false;
+}


you shouldn't do that, because it won't trigger any react re-render on css load.

Instead do something like I suggested:

import "./hideCSSLoadingWarning.css"

.css-loading-warning { display: none; }

If the CSS of the site loads, the message will automatically hide, so it will remain visible only if css does not load.

Oh now I get it!
Sorry I think I had misunderstood it the first time.
I'll try this, thank you!

slorber · 2020-10-23T14:55:10Z

packages/docusaurus/src/client/App.tsx

+      <div style={{border: 'solid red thick', padding: '16px'}}>
+        <span>
+          You site CSS did not load properly. Your baseUrl setting is probably
+          bad.


Still think we need a suggestion here. if we can tell the user the exact value he should use it's better.

Agreed!
Maybe I can do something like location.pathname.split('/')[0] ?

You had also mentioned "only display this message on the homepage".
So should I maybe move this to somewhere else?

slorber · 2020-10-27T10:32:41Z

As the browser won't load the js/css bundles in case of bad baseUrl configuration, we'll likely need a better way to display this suggestion.

The JS should be inlined in the static HTML files, so that it can always execute even if the css/js bundles don't load.

Here

import "./hideCSSLoadingWarningMessage.css";

function CSSLoadingWarningMessage() {
  const location = useLocation();
  if (location.pathname === baseUrl) {
    return (
      <div style={{ border: "solid red thick" }}>
        <div>CSS didn't load</div>
        <div>Mabe you should try baseurl=<span className="css-didnt-load-baseurl-suggestion"/></div>
        <script>
          document.querySelector("span").text = window.location.pathname
        </script> 
      </div>
    );
  }
  return null;
}

Here is pseudo code for inspiration, not sure it works

jcs98 · 2020-10-27T18:14:01Z

As the browser won't load the js/css bundles in case of bad baseUrl configuration, we'll likely need a better way to display this suggestion.

The JS should be inlined in the static HTML files, so that it can always execute even if the css/js bundles don't load.

Here
import "./hideCSSLoadingWarningMessage.css";

function CSSLoadingWarningMessage() {
  const location = useLocation();
  if (location.pathname === baseUrl) {
    return (
      <div style={{ border: "solid red thick" }}>
        <div>CSS didn't load</div>
        <div>Mabe you should try baseurl=<span className="css-didnt-load-baseurl-suggestion"/></div>
        <script>
          document.querySelector("span").text = window.location.pathname
        </script> 
      </div>
    );
  }
  return null;
}
Here is pseudo code for inspiration, not sure it works

Thank you!
Inlining the script in the JSX did not work. I added that js to the ssr template which seems to work. Please let me know if that is okay.

I could not get the baseUrl using const baseUrl = useBaseUrl('');. Can you please tell me how to get it using site config.
I also think if the baseUrl is set incorrectly which causes the problem in the first place, then using it in if (location.pathname === baseUrl) might not be helpful.

So as of now, the warning div works (it is hidden whenever css loads successfully) and the suggestion message works too.

slorber · 2020-10-28T17:12:40Z

I'd really like the script to be encapsuled in the warning component, so that the feature is easier to maintain (not splitted across the codebase) + also I don't want this script to be appended to all HTML pages, only the homepage.

What didn't work exactly?
Can you commit your attempt so that I can inspect the deploy preview and see what's going on?
Have you tried using @docusaurus/Head?
Maybe it's required to add the script to <head> (+ DOMContentLoaded event) and maybe React prevent the inline script to be added to body. Unfortunately I can't tell because I can't see your failed attempt and what output it produces.

You should not use useBaseUrl('');, it's not meant to get the base URL, it's meant to add the baseurl prefix to links.
You can get the config baseurl from docusaurus context: https://v2.docusaurus.io/docs/docusaurus-core#usedocusauruscontext

And yes, if baseurl is wrongly configured, it will be a bad baseurl. But on SSR, we also use that bad baseurl to render the pages on a static location, so useLocation().pathname === useDocusaurusContext().siteConfig.baseUrl is likely a good way to detect if we are server-rendering the homepage (but it would only return true on the server, which should be good enough for our case)

Please try my suggestions, commit and ping me so that I can inspect the code and Netlify deployment. Only once we agree the approach does not work, it's worth exploring another approach, but I think it should work.

If you need help I can commit on your branch

slorber · 2020-10-28T17:16:25Z

packages/docusaurus/src/client/templates/ssr.html.template.js

@@ -35,6 +35,10 @@ module.exports = `
      <script type="text/javascript" src="<%= it.baseUrl %><%= script %>"></script>
    <% }); %>
    <%~ it.postBodyTags %>
+    <script>
+      document.getElementById('css-didnt-load-baseurl-suggestion').innerHTML =


We'd rather be "defensive" and consider the element might not be found, in such case we should not have any console error

Also I prefer to not have this script in the template as it splits the feature in multiple parts, affect the output of all the pages (not just the homepage) and will lead to a bad suggestion if user tries to browse another non-home page of the broken site (ie, suggestion /baseUrl/docs/mydoc if user is browsing /docs/mydoc)

jcs98 · 2020-10-29T11:05:49Z

Thank you @slorber
Using baseUrl from siteConfig worked.

I also tried using @docusaurus/Head but that does not seem to work if the baseUrl is configured incorrectly.
Can you please see if I'm doing something wrong? I've added my attempt to this PR.

slorber · 2020-10-30T16:33:05Z

I see, the comp did not inject properly the scripts inside the homepage due to a missing little thing.

Completed your PR and polished it, going to merge soon ;)

slorber · 2020-10-30T17:01:00Z

Seems to work in all cases.

Added a trailing slash to baseurl suggestion, as it's required by the config validation.

slorber · 2020-10-30T17:43:28Z

thanks for your work :)

jcs98 · 2020-10-30T18:24:56Z

Thank you for your guidance on this @slorber! 😄

jcs98 requested review from lex111 and slorber as code owners October 21, 2020 13:24

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 21, 2020

jcs98 marked this pull request as draft October 21, 2020 13:39

slorber changed the title ~~Show big help message when there are baseurl problems~~ feat(v2): Show big help message when there are baseurl problems Oct 21, 2020

slorber added the pr: new feature This PR adds a new API or behavior. label Oct 21, 2020

jcs98 force-pushed the jcs98/css-warning branch from 22c56e5 to 8dadd7a Compare October 22, 2020 10:40

slorber requested changes Oct 22, 2020

View reviewed changes

jcs98 force-pushed the jcs98/css-warning branch from 8dadd7a to 4dead0f Compare October 22, 2020 17:04

jcs98 marked this pull request as ready for review October 22, 2020 17:08

slorber requested changes Oct 23, 2020

View reviewed changes

jcs98 force-pushed the jcs98/css-warning branch from 4dead0f to 80491d8 Compare October 25, 2020 07:19

jcs98 force-pushed the jcs98/css-warning branch from 80491d8 to c25ca7d Compare October 27, 2020 17:53

slorber reviewed Oct 28, 2020

View reviewed changes

Add CSSLoadingWarningMessage to App in client

04e91f6

jcs98 force-pushed the jcs98/css-warning branch from c25ca7d to 04e91f6 Compare October 29, 2020 10:54

slorber added 2 commits October 30, 2020 17:36

fix/polish BaseUrlSuggestionWarning

9d99b06

add baseUrl trailing slash + polish

e053743

slorber changed the title ~~feat(v2): Show big help message when there are baseurl problems~~ feat(v2): baseUrl config issues: show help message if css/js can't load Oct 30, 2020

slorber merged commit 3a8bad2 into facebook:master Oct 30, 2020

slorber linked an issue Nov 4, 2020 that may be closed by this pull request

DX: show big help message when baseurl problems #3584

Closed

lex111 added this to the v2.0.0-alpha.67 milestone Nov 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(v2): baseUrl config issues: show help message if css/js can't load #3621

feat(v2): baseUrl config issues: show help message if css/js can't load #3621

jcs98 commented Oct 21, 2020

docusaurus-bot commented Oct 21, 2020 •

edited

Loading

slorber left a comment

slorber Oct 22, 2020

jcs98 Oct 22, 2020

slorber Oct 22, 2020

jcs98 Oct 22, 2020

jcs98 commented Oct 22, 2020 •

edited

Loading

slorber left a comment •

edited

Loading

slorber Oct 23, 2020

jcs98 Oct 23, 2020

slorber Oct 23, 2020

jcs98 Oct 23, 2020

slorber commented Oct 27, 2020

jcs98 commented Oct 27, 2020

slorber commented Oct 28, 2020 •

edited

Loading

slorber Oct 28, 2020 •

edited

Loading

jcs98 commented Oct 29, 2020

slorber commented Oct 30, 2020

slorber commented Oct 30, 2020

slorber commented Oct 30, 2020

jcs98 commented Oct 30, 2020

	<CSSLoadingWarningMessage isClient />
	<CSSLoadingWarningMessage isClient={isClient} />

feat(v2): baseUrl config issues: show help message if css/js can't load #3621

feat(v2): baseUrl config issues: show help message if css/js can't load #3621

Conversation

jcs98 commented Oct 21, 2020

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

docusaurus-bot commented Oct 21, 2020 • edited Loading

slorber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcs98 commented Oct 22, 2020 • edited Loading

slorber left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slorber commented Oct 27, 2020

jcs98 commented Oct 27, 2020

slorber commented Oct 28, 2020 • edited Loading

slorber Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

jcs98 commented Oct 29, 2020

slorber commented Oct 30, 2020

slorber commented Oct 30, 2020

slorber commented Oct 30, 2020

jcs98 commented Oct 30, 2020

docusaurus-bot commented Oct 21, 2020 •

edited

Loading

jcs98 commented Oct 22, 2020 •

edited

Loading

slorber left a comment •

edited

Loading

slorber commented Oct 28, 2020 •

edited

Loading

slorber Oct 28, 2020 •

edited

Loading