Fixing Issue 305 - broken links because of language #316

richardzcode · 2017-12-19T02:08:57Z

Motivation

This is to fix #305

The root cause of the problem is using 'en' as default language regardless of translation enabled or not.

However 'en' and no translation are two different cases. The fix has two major parts:

Components. Basically use empty language property as an indication of no translation.
Template generation. Basically handle the case of no translation.
For readability I've created a new file env.js which encapsulate simple translation and versioning logic. So other places can just call env.translation.enabled to check.

I try to keep the changes minimal but sill has to modify quite a few files.

Test Plan

I've tested run server and generate locally.

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

Let me know if double having is intentional

JoelMarcey · 2017-12-19T02:49:08Z

@richardzcode Hi! Yep the build failed because prettier issues thus far.

Did you run npm run prettier on your pull request? Here is the changes that Prettier is recommending.

https://gist.github.com/JoelMarcey/215ea77e546496ab5758bcc86fe4152c

richardzcode · 2017-12-19T03:37:56Z

My fault. I did run prettier but did not check what prettier changed... Let me try again.

JoelMarcey · 2017-12-19T03:42:36Z

....and tests passed :)

richardzcode · 2017-12-19T03:50:15Z

😁

JoelMarcey

@richardzcode I really like this pull request. I understand what you are trying to do here, and I think it makes sense.

I have some feedback/questions.

One key thing is that I think you need to rebase to the latest in master because some of the code I see in your PR is stuff that you are bringing back that has been removed in other PRs.

I want to think about this some more, and maybe let some of the other core team have a look as well. I may try to run this on a couple of our other sites too to make sure we are not breaking anyone horribly.

But I am impressed. <3 👍

JoelMarcey · 2017-12-19T03:57:13Z

lib/server/readMetadata.js

@@ -81,8 +68,7 @@ function readSidebar() {

 // split markdown header
 function splitHeader(content) {
-  // New line characters need to handle all operating systems.
-  const lines = content.split(/\r?\n/);


I think we want to keep the content.split(/\r?\n/); so we can continue to support, Windows, right? This was just added in #304 by @neilsutcliffe

JoelMarcey · 2017-12-19T03:57:33Z

lib/core/nav/HeaderNav.js

        ? this.props.language + '/versions.html'
        : 'versions.html');
    return (
      <div className="fixedHeaderContainer">
        <div className="headerWrapper wrapper">
          <header>
            <a href={this.props.baseUrl}>
-              {siteConfig.headerIcon && (


Did you mean to remove the siteConfig.headerIcon check? It was just added in #312.

JoelMarcey · 2017-12-19T04:01:06Z

lib/server/readMetadata.js

@@ -149,28 +132,19 @@ function processMetadata(file) {
    metadata.title = metadata.id;
  }

-  if (languages.length === 1 && !siteConfig.useEnglishUrl) {


So, siteConfig.useEnglishUrl is a config option (that is not documented, unfortunately - but I will document it if we agree), that allows one to override the default of /docs/doc.html with something like /docs/en/doc.hmtl.

Are you thinking we shouldn't allow this override any longer?

my mistake. should keep this override. should we use this override, or just have a languages.js with only 'en' enabled. It is a separate question.

My personal opinion is that we document and use the override to keep a clean separation between sites that have translations and sites that do not. If you have a language.js file, then translations will be enabled. If you want the /docs/en format, but no translations, you can use this override.

Seem reasonable?

JoelMarcey · 2017-12-19T04:03:56Z

lib/server/readMetadata.js

@@ -194,6 +170,8 @@ function processMetadata(file) {

 // process metadata for all docs and save into core/metadata.js
 function generateMetadataDocs() {
+  console.log('Generating Metadata for Docs....');


I think you may not be fully rebased to the latest master. I know this was removed on purpose because the logs were too noisy in #291

JoelMarcey · 2017-12-19T04:04:08Z

lib/server/readMetadata.js

@@ -345,6 +322,13 @@ function generateMetadataBlog() {
  const metadatas = [];

  let files = glob.sync(CWD + '/blog/**/*.*');
+  if (!files || files.length == 0) {


Same here. This was also removed in #291

JoelMarcey · 2017-12-19T04:04:32Z

lib/server/server.js

@@ -122,6 +120,8 @@ function execute(port) {

  /****************************************************************************/

+  console.log('server.js triggered...');


Same here too: #291

JoelMarcey · 2017-12-19T04:06:00Z

lib/server/generate.js

  const siteConfig = require(CWD + '/siteConfig.js');
  const translate = require('./translate.js');
-  const versionFallback = require('./versionFallback.js');


I am assuming this is removed because it is unused.

yes this one is not used.

JoelMarcey · 2017-12-19T04:06:13Z

lib/server/server.js

@@ -23,15 +24,12 @@ function execute(port) {
  const glob = require('glob');
  const chalk = require('chalk');
  const translate = require('./translate.js');
-  const versionFallback = require('./versionFallback');


I am assuming this is removed because it is unused.

yes this is unused.

JoelMarcey · 2017-12-19T04:06:27Z

lib/server/server.js

@@ -523,6 +518,7 @@ function execute(port) {
  });

  app.listen(port);
+  console.log('listening on port: ' + port);


Same here too: #291

JoelMarcey · 2017-12-19T04:10:04Z

@ericnakagawa @ericvicenti @hramos - maybe have a quick look at this too to make sure I am not missing anything key here. I like the PR.

…into issue-305

richardzcode · 2017-12-19T07:01:17Z

Thank you @JoelMarcey for the thorough review! Not sure what went wrong with my rebase. Now corrected. I'll be extra careful next time.

hramos

LGTM

JoelMarcey

👍

I am going to go with this. Thanks so much for this -- this definitely wasn't an easy task.

This reverts commit 7dc6c6c.

JoelMarcey · 2017-12-19T17:25:42Z

I had to revert this actually -- I just noticed a link in docusaurus.io that was broken. Let me check out why.

JoelMarcey · 2017-12-19T18:37:08Z

@richardzcode Hi. Sorry I had to revert this.

However, I cherry-picked your pull request locally and am running some tests on it. I am finding some things, which I will comment inline.

JoelMarcey · 2017-12-19T18:38:19Z

lib/server/env.js

+const join = path.join;
+
+const languages_js = join(CWD, 'lanauages.js');
+const translation_js = join(CWD, 'translation.js');


This needs to be const translation_js = './translation.js; since this file is in the same directory as env.js, not as part of the current process' working directory.

JoelMarcey · 2017-12-19T18:38:48Z

lib/server/env.js

+
+const join = path.join;
+
+const languages_js = join(CWD, 'lanauages.js');


This was spelled wrong :)

languages.js

JoelMarcey · 2017-12-19T18:39:43Z

lib/core/nav/HeaderNav.js

-      }
+      const langPart = env.translation.enabled ? this.props.language + '-' : '';
+      const versionPart =
+        env.translation.enabled && this.props.version !== 'next'


This should be env.versioning.enabled, not env.translation.enabled.

JoelMarcey · 2017-12-19T18:40:16Z

lib/server/generate.js

@@ -443,21 +425,25 @@ function execute() {
            str
          );
        }
+
+        // write to base level


I am thinking there is something funny going on in this section.

But I am not 100% sure yet.

This was a change I made in order to generate default pages from English pages. Original code was to copy English pages to one level above. However those pages has {language} property as 'en' which causes links wrong. So I made the change to generate instead. Couldn't think of a problem it would cause, but let me run some tests first.

richardzcode · 2017-12-19T18:58:56Z

Sorry about the mistakes. Let me run some more tests. Should I continue fix on this PR? Or it should be handled separately?

JoelMarcey · 2017-12-19T19:00:26Z

No apologies necessary :) I really appreciate the work here. I think we are close.

Since I merged this one already, it's hard to bring back. Maybe a new PR with the fixes in it would be best. Is that ok?

JoelMarcey · 2017-12-19T19:03:24Z

@richardzcode When I try to build docusaurus locally using npm run build within it's website directory, after those small fixes I let you know about, I get the following error:

/Users/joelm/dev/Docusaurus-primary/lib/core/nav/HeaderNav.js:175
            throw new Error("It looks like you've enabled language support, but haven't provided translated files. The document with id: '" + id + "' doesn't exist.");
            ^

Error: It looks like you've enabled language support, but haven't provided translated files. The document with id: '-installation' doesn't exist.

And that is because at some point this.props.language becomes undefined.

richardzcode · 2017-12-19T19:06:12Z

sure. Working on it.

richardzcode · 2017-12-19T19:14:31Z

I think I know what's going on in HeaderNav.js.

This line:

const langPart = env.translation.enabled ? this.props.language + '-' : '';

Because we are generating default pages with language=''. When translation enabled, langPart would become '-doc1' etc. which does not exist in Metadata.

But I am confused why my local test doesn't have the problem...

JoelMarcey · 2017-12-19T20:37:16Z

@richardzcode Are you testing on your own personal site? Or are you testing on the Docusaurus site itself?

richardzcode · 2017-12-19T21:31:27Z

#322

richardzcode · 2017-12-19T21:33:53Z

@JoelMarcey Testing on my own site. You are right. I should test on Docusaurus site.

Need this because of facebook#316

Need this because of #316

richardzcode added 4 commits December 14, 2017 12:03

without having having to worry about site design.

b20e1af

Let me know if double having is intentional

Merge branch 'master' of https://github.com/richardzcode/Docusaurus

dbb19a1

Merge branch 'master' of https://github.com/richardzcode/Docusaurus

4e8cece

distinguish case of no translation and en lang

499aff0

facebook-github-bot added the CLA Signed Signed Facebook CLA label Dec 19, 2017

prettier recommends

1cdffe2

JoelMarcey self-requested a review December 19, 2017 03:45

JoelMarcey suggested changes Dec 19, 2017

View reviewed changes

richardzcode added 4 commits December 18, 2017 22:12

distinguish case of no translation and en lang

04a0e26

prettier recommends

90818c5

Merge branch 'issue-305' of https://github.com/richardzcode/Docusaurus …

4e48aa3

…into issue-305

merge with latest origin/master changes

2a6ca41

typo

8a6ca46

hramos approved these changes Dec 19, 2017

View reviewed changes

JoelMarcey approved these changes Dec 19, 2017

View reviewed changes

JoelMarcey merged commit 7dc6c6c into facebook:master Dec 19, 2017

JoelMarcey added a commit that referenced this pull request Dec 19, 2017

Revert "Fixing Issue 305 - broken links because of language (#316)"

18c9a2a

This reverts commit 7dc6c6c.

JoelMarcey mentioned this pull request Dec 19, 2017

Broken links in example index.js because of language #305

Closed

JoelMarcey reviewed Dec 19, 2017

View reviewed changes

JoelMarcey added a commit to JoelMarcey/Docusaurus that referenced this pull request Dec 27, 2017

Check English translation if language doesn't exist

faace5a

Need this because of facebook#316

This was referenced Dec 27, 2017

Check English translation if language doesn't exist #361

Merged

Do not set html lang attribute if there is no language set #362

Merged

JoelMarcey added a commit that referenced this pull request Dec 27, 2017

Check English translation if language doesn't exist (#361)

5a77f18

Need this because of #316

		@@ -122,6 +120,8 @@ function execute(port) {

		/****************************************************************************/

		console.log('server.js triggered...');


		const join = path.join;

		const languages_js = join(CWD, 'lanauages.js');

Fixing Issue 305 - broken links because of language #316

Fixing Issue 305 - broken links because of language #316

Conversation

richardzcode commented Dec 19, 2017

Motivation

Test Plan

Related PRs

JoelMarcey commented Dec 19, 2017

richardzcode commented Dec 19, 2017

JoelMarcey commented Dec 19, 2017

richardzcode commented Dec 19, 2017

JoelMarcey 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

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

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

Choose a reason for hiding this comment

JoelMarcey commented Dec 19, 2017

richardzcode commented Dec 19, 2017

hramos left a comment

Choose a reason for hiding this comment

JoelMarcey left a comment

Choose a reason for hiding this comment

JoelMarcey commented Dec 19, 2017

JoelMarcey commented Dec 19, 2017

JoelMarcey Dec 19, 2017 • 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

Choose a reason for hiding this comment

richardzcode commented Dec 19, 2017

JoelMarcey commented Dec 19, 2017

JoelMarcey commented Dec 19, 2017

richardzcode commented Dec 19, 2017

richardzcode commented Dec 19, 2017 • edited Loading

JoelMarcey commented Dec 19, 2017

richardzcode commented Dec 19, 2017

richardzcode commented Dec 19, 2017

JoelMarcey Dec 19, 2017 •

edited

Loading

richardzcode commented Dec 19, 2017 •

edited

Loading