refactor: readMetadata & env.localization #907

SleepWalker · 2018-08-19T18:57:06Z

Motivation

This PR appeared during during my work on #868. It contains no new features or logic changes. I've rearranged the code to decouple it from the environment and be able to cover it with tests.

The docs-related logic from server/readMetadata was moved into a separate class DocsMetadata and covered with test.

I've also removed translations.js and replaced it with Translation.t(language, key, category) interface and moved getLanguage implementation from utils to Translation. The Translation class was moved from server/env into a separate file

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

The best way is to compare metadata.js before and after my refactoring applied (e.g. with jsondiff.com). They should be the same. The problem is that I can not test with translations applied. I've compared only the metadata.js files I can get locally.

Another one option is to compare current gh-pages version and netlify version using different locales and doc versions.

docusaurus-bot · 2018-08-19T18:59:02Z

Deploy preview for docusaurus-preview ready!

Built with commit 046070b

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

docusaurus-bot · 2018-08-19T19:01:02Z

Deploy preview for docusaurus-preview ready!

Built with commit d5e5aee

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

endiliey

I like the idea of removing translations.js and replacing it with Translation.t(language, key, category) interface but unfortunately removing translation.js will be causing breaking changes to user and we want to avoid that for now because v2 will have breaking changes as well 😄

Reasonml for example use translation
https://github.com/reasonml/reasonml.github.io/blob/42e9a33610244fbf84838cbd29c02d8f111cf11e/website/pages/en/index.js#L9

const translation = require('../../server/translation.js');

Can you revert it back and I'll continue to review ?

SleepWalker · 2018-08-23T08:07:57Z

@endiliey ok. I can restore that file, but probably it would be better to add a deprecation warning in that file?

If the user will access the data through the interface it will allow Docusaurus developers to safely change the way that data handled.

Regarding Translation.t(language, key, category). I think that in future it will be possible to set the current locale for Translation, so the first argument may be optional in future. Considering this I can implement the logic that will support following signatures:

Translation.t(key, language)
Translation.t(key, category, language)

This will make internals of Translate.t more complicated, but in future we will be able to migrate to the following signatures:

Translation.t(key)
Translation.t(key, category)

endiliey · 2018-08-23T08:18:37Z

Hmm.. I like the idea of that but I think I need extra opinion on this. Cc @yangshun @JoelMarcey

JoelMarcey · 2018-08-24T03:52:46Z

Yeah, we would definitely have to start with a deprecation and give a few releases and warnings in the release notes about it since if any project that uses translations would require the current file. But I think that is ok.

SleepWalker · 2018-09-02T14:16:40Z

@endiliey the translation.js is back and exports Translation instance. I have also removed language argument to translation.t. Now it will use the language, that will be set before rendering.

When some one will try to use translation[langId] the warning will be printed in console.

I have merged with current master and added support for keys as array, that was introduced in #917, e.g.:

    const title = translation.t(['docs', id, 'title']) || defaultTitle;

endiliey

This is a huge change 😲

Overall it looks good to me, but definitely we'll have to keep on reviewing & this need more than just my review.

cc @yangshun @JoelMarcey

Another thing is that this will definitely clash/conflicts with #892. Whichever got merged first 😬

endiliey · 2018-09-02T15:41:16Z

lib/server/generate.js

@@ -386,6 +389,8 @@ async function execute() {
      const pageID = path.basename(normalizedFile, '.html');
      const parts = normalizedFile.split('pages');
      const targetFile = join(buildDir, parts[1]);
+      env.translation.setLanguage('en');


There are some codes in this file that use translate.setLanguage(language); and sometimes we use env.translation.setLanguage() although its pointing to the same logic. Shouldn't we go for consistency ?

You are right. I'll fix it.

Can I safely remove setLanguage from server/translate.js? Or should I add deprecation warning there?

https://docusaurus.io/docs/en/translation#enabling-translations-on-pages

It seems safe to remove

endiliey · 2018-09-02T16:25:07Z

lib/server/env/Translation.js

+      .replace(new RegExp('\\\\', 'g'), '\\');
+  }
+
+  resolveMessage(translations, keyParts) {


I think this is too generic to be put inside translation.

resolveMessage(translations, keyParts) { return keyParts.reduce((obj, key) => obj && obj[key], translations); }

We should preserve the safe-getter idx function in core/utils.js to access deeply nested object and this class can instead import that utility function

endiliey · 2018-09-02T16:36:39Z

lib/server/blog.js

  const blogPages = {};
  const perPage = 10;
+
+  env.translation.setLanguage('en');


Do we need this ? There is no translation in blog

But there are translations in <Site> component. I think it is safer to explicitly set language to default en, because it may happen, that current translation language will not be 'en' due to some doc page rendered previously using other language.

SleepWalker · 2018-09-02T18:09:38Z

Another thing is that this will definitely clash/conflicts with #892. Whichever got merged first

I have also finished with #868 and #827, which are improvements of sidebar too. I haven't submitted PR, because I'm coding on base of current PR.

I can wait till #892 merged and then resolve all the conflicts or if my changes are too big, I can drop them and try to resolve 868 and 827 without any refactoring.

endiliey · 2018-09-02T18:25:39Z

I am more inclined to merge this before #892 actually.

We are going for rewrite in v2 so I favor this refactoring and test more than feature.

However I am not confident to merge such big change alone.

Current architecture has a lot of circular require(), for example the env & siteConfig.js.

Ideally we want v2 to have unidirectional data flow.

* Add method to access localized strings * Add deprecation warning for old api usage * Implement translation fallbacks * Ensure setLanguage called before any rendering

endiliey · 2018-09-14T18:04:39Z

Hey @SleepWalker,

Sorry for another late response. First of all, you might want to see https://docusaurus.io/blog/2018/09/11/Towards-Docusaurus-2

due to work on Docusaurus 2, we will be less likely to accept new features/major changes on Docusaurus 1.

I personally prefer not to merge this since this part of docs metadata codes has been re-written & changed on v2 into pure functions without CWD/ global require anymore (no more side effects, yay !!) with > 90% test coverage.

However, will you be interested in helping for v2 instead? I'll notify you when we've moved part of the work on this repository's v2 branch. I'm sure you'll be a great help 😃

SleepWalker · 2018-09-15T10:09:07Z

Hi, @endiliey

yep. I've read that post and it would be interesting to help with the v2 :)

I'm closing this PR

endiliey · 2018-09-27T09:39:47Z

Hi @SleepWalker. I think this can now be done in v2. You might want to checkout v2 folder (although its still wip)

Feel free to ping me on GitHub or discord

facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 19, 2018

SleepWalker force-pushed the sidebar-metadata-refactoring branch from 046070b to 20664ff Compare August 19, 2018 18:57

SleepWalker force-pushed the sidebar-metadata-refactoring branch from 20664ff to 43914b7 Compare August 21, 2018 04:37

endiliey self-assigned this Aug 22, 2018

endiliey added the ✅ test plan label Aug 22, 2018

endiliey mentioned this pull request Aug 22, 2018

feat: sub-categories in sidebar.json #892

Merged

endiliey suggested changes Aug 23, 2018

View reviewed changes

endiliey removed their assignment Aug 23, 2018

endiliey changed the title ~~Refactor readMetadata and env.localization~~ refactor: readMetadata & env.localization Aug 23, 2018

JoelMarcey requested a review from yangshun August 24, 2018 03:47

endiliey reviewed Sep 2, 2018

View reviewed changes

SleepWalker force-pushed the sidebar-metadata-refactoring branch 3 times, most recently from a5d41ae to ab196d2 Compare September 5, 2018 18:01

SleepWalker added 3 commits September 5, 2018 21:15

Decouple sidebar index creation logic

cdcc3e2

Move getLanguage implementation into a Translation class.

2c4820c

Refactor sidebar metadata reading and cover with tests

dc7e3aa

SleepWalker force-pushed the sidebar-metadata-refactoring branch 4 times, most recently from cbecd77 to b4c564e Compare September 5, 2018 18:53

SleepWalker force-pushed the sidebar-metadata-refactoring branch 4 times, most recently from 812b2f1 to 7342942 Compare September 5, 2018 19:38

Merge translation.js into env/Translation

d5e5aee

* Add method to access localized strings * Add deprecation warning for old api usage * Implement translation fallbacks * Ensure setLanguage called before any rendering

SleepWalker force-pushed the sidebar-metadata-refactoring branch from 7342942 to d5e5aee Compare September 5, 2018 19:58

SleepWalker closed this Sep 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: readMetadata & env.localization #907

refactor: readMetadata & env.localization #907

SleepWalker commented Aug 19, 2018

docusaurus-bot commented Aug 19, 2018

docusaurus-bot commented Aug 19, 2018 •

edited

Loading

endiliey left a comment •

edited

Loading

SleepWalker commented Aug 23, 2018

endiliey commented Aug 23, 2018

JoelMarcey commented Aug 24, 2018

SleepWalker commented Sep 2, 2018

endiliey left a comment

endiliey Sep 2, 2018

SleepWalker Sep 2, 2018

endiliey Sep 2, 2018

endiliey Sep 2, 2018 •

edited

Loading

endiliey Sep 2, 2018

SleepWalker Sep 2, 2018

SleepWalker commented Sep 2, 2018

endiliey commented Sep 2, 2018 •

edited

Loading

endiliey commented Sep 14, 2018

SleepWalker commented Sep 15, 2018

endiliey commented Sep 27, 2018

refactor: readMetadata & env.localization #907

refactor: readMetadata & env.localization #907

Conversation

SleepWalker commented Aug 19, 2018

Motivation

Have you read the Contributing Guidelines on pull requests?

Test Plan

docusaurus-bot commented Aug 19, 2018

docusaurus-bot commented Aug 19, 2018 • edited Loading

endiliey left a comment • edited Loading

Choose a reason for hiding this comment

SleepWalker commented Aug 23, 2018

endiliey commented Aug 23, 2018

JoelMarcey commented Aug 24, 2018

SleepWalker commented Sep 2, 2018

endiliey left a comment

Choose a reason for hiding this comment

endiliey Sep 2, 2018

Choose a reason for hiding this comment

SleepWalker Sep 2, 2018

Choose a reason for hiding this comment

endiliey Sep 2, 2018

Choose a reason for hiding this comment

endiliey Sep 2, 2018 • edited Loading

Choose a reason for hiding this comment

endiliey Sep 2, 2018

Choose a reason for hiding this comment

SleepWalker Sep 2, 2018

Choose a reason for hiding this comment

SleepWalker commented Sep 2, 2018

endiliey commented Sep 2, 2018 • edited Loading

endiliey commented Sep 14, 2018

SleepWalker commented Sep 15, 2018

endiliey commented Sep 27, 2018

docusaurus-bot commented Aug 19, 2018 •

edited

Loading

endiliey left a comment •

edited

Loading

endiliey Sep 2, 2018 •

edited

Loading

endiliey commented Sep 2, 2018 •

edited

Loading