Add doctype to generated html files. #382
Conversation
Actually all generated html files doesn't have a doctype. In order to fix this, a doctype has been added when renderToStaticMarkup function is invoked. Accordingly to theses changes, a test has been added in order to test that all html files contains a doctype. BxooeforeAll function has been updated to in order to ensure that we targeted all html files, even ones which are in subdirectory like /docs/en/*.html.
There was a problem hiding this comment.
@Tonours Thank you for the pull request! I think this is reasonable, but I have some questions.
| @@ -34,7 +34,7 @@ beforeAll(() => { | |||
| generateSite(); | |||
| return Promise.all([ | |||
| glob(docsDir + '/**/*.md'), | |||
| glob(buildDir + '/' + siteConfig.projectName + '/docs/*.html'), | |||
| glob(buildDir + '/' + siteConfig.projectName + '/docs/**/*.html'), | |||
There was a problem hiding this comment.
Why did you make this change here?
There was a problem hiding this comment.
Hi @JoelMarcey ! Thanks for your feedback :-)
This change has been made to include html files which are in subdirectories like /docs/en/about-slash.html and not just /docs/about-slash.html, to ensure that not only html files with redirect have a doctype.
| @@ -180,7 +180,7 @@ function execute() { | |||
| {rawContent} | |||
| </DocsLayout> | |||
| ); | |||
| const str = renderToStaticMarkup(docComp); | |||
| const str = '<!DOCTYPE html>' + renderToStaticMarkup(docComp); | |||
There was a problem hiding this comment.
Is there a reason why you could not just put this in the render section of the site in Site.js, so you only have to do it one time?
Right above here, before the <html> tag? https://github.com/facebook/Docusaurus/blob/master/lib/core/Site.js#L56
There was a problem hiding this comment.
It would be great yes, but React seems to not allow us do to this and raised an error about Doctype, here is an issue with this point on React repo facebook/react#1035.
There was a problem hiding this comment.
@Tonours Ugh. That's unfortunate. So it might be that your way is the only/best way to do it - injecting <DOCTYPE html> inline with any call to renderToStaticMarkup.
@rickyvetter would you know a better way to do this given what we are seeing in facebook/react#1035
There was a problem hiding this comment.
This is quite a verbose way of injecting it even though the issue suggests that string manipulation is the only way of doing this. How about wrapping renderToStaticMarkup to keep it DRY?
const renderToStaticMarkup = require('react-dom/server').renderToStaticMarkup;
...
// Some better name (the hard things in coding...):
const fullyRenderToStaticMarkup = (...args) => `<!DOCTYPE html>${renderToStaticMarkup(...args)}`;NB: Could replace destructuring (...args) with explicit arguments if not supported or type inference is needed at some point
|
@Tonours Any update on this? Do let us know if you're still interested in patching this PR, else we can take over. Let's create a new module // render-utils.js
const renderToStaticMarkup = require('react-dom/server').renderToStaticMarkup;
/**
* Custom function that wraps renderToStaticMarkup so that we can inject
* HTML code before React renders the contents.
*/
function renderToStaticMarkupWithDoctype(...args) {
return '<!DOCTYPE html>' + renderToStaticMarkup(...args);
}
module.exports = {
renderToStaticMarkupWithDoctype,
};and replace all existing callsites to |
|
@yangshun Hey ! Yes you can take over. |
|
Closed by #566 |
Motivation
Notice missing doctype for html generated files.
Related issue: #383
Test Plan
With Jest
Run unit test with
yarn run test, a new test has been added to ensure that generated files have a doctype.Unit test screenshot
Manually
Otherwhise, build the project in
websitedirectory withyarn run buildand go to/website/builddirectory in order to manually check that html files starts with a doctype.Manually build test screenshot