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(core): allow customizing the i18n directory path #7386

Merged
merged 6 commits into from
Jun 2, 2022
Merged

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented May 10, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Requested by @slorber.

In addition to adding the i18n.path config option, this PR also made an internal refactor (namely, adding context. localizationDir ) to make the future feature of per-locale path customization easier.

I did not include the per-locale path customization in this PR, to make it easier to review since it's almost like a refactor (getting rid of the I18N_DIR constant).

Test Plan

Updated all existing tests

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

@Josh-Cena Josh-Cena added the pr: new feature This PR adds a new API or behavior. label May 10, 2022
@Josh-Cena Josh-Cena requested review from slorber and lex111 as code owners May 10, 2022 09:39
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 10, 2022
@netlify
Copy link

netlify bot commented May 10, 2022

[V2]

Name Link
🔨 Latest commit ad28fd5
🔍 Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/62988e248125380009713f31
😎 Deploy Preview https://deploy-preview-7386--docusaurus-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions
Copy link

github-actions bot commented May 10, 2022

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 70 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 80 🟢 99 🟢 100 🟢 100 🟢 90 Report

@github-actions
Copy link

github-actions bot commented May 10, 2022

Size Change: +26 B (0%)

Total Size: 798 kB

ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 52.6 kB 0 B
website/build/assets/css/styles.********.css 106 kB 0 B
website/build/assets/js/main.********.js 600 kB +26 B (0%)
website/build/index.html 38.9 kB 0 B

compressed-size-action

@Josh-Cena Josh-Cena force-pushed the jc/i18n-path branch 2 times, most recently from 908f083 to d883c97 Compare May 14, 2022 16:05
@Josh-Cena Josh-Cena force-pushed the jc/i18n-path branch 3 times, most recently from 90eda99 to 4199ef3 Compare May 25, 2022 03:49
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

What prevents us from moving i18nDir in the ì18n type?

@@ -100,6 +100,7 @@ export async function loadContext(
return {
siteDir,
generatedFilesDir,
i18nDir,
Copy link
Collaborator

Choose a reason for hiding this comment

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

wonder if we can't move this under the I18n object directly instead of introducing a new context key?

export type I18n = DeepRequired<I18nConfig> & {currentLocale: string; currentLocalePath: string};

?

Copy link
Collaborator Author

@Josh-Cena Josh-Cena May 27, 2022

Choose a reason for hiding this comment

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

Because we have siteDir, generatedFilesDir , and outDir, it makes sense to put i18nDir here? (On second thought, I think we should even have a new staticDirs context here!)

More pratically, I18n is accessible to the client, but the client shouldn't be aware of the file path, so it's just excess bundle size generated. It's also not part of the user config (unlike i18n.path)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because we have siteDir, generatedFilesDir , and outDir, it makes sense to put i18nDir here?

My idea is that if we can namespace related things together it will be better in the long term? This will be used by plugin authors

I think we should even have a new staticDirs context here

yes we'll likely want that 👍

More pratically, I18n is accessible to the client, but the client shouldn't be aware of the file path

We could filter it in the emitted client file if needed

It's also not part of the user config (unlike i18n.path)

Yes, but we'll likely want to make the per-locale dir configurable too later, as it's not flexible enough to expect localization dirs to be all in the i18n dir. Some users would prefer to have a _fr folder at the root of their monorepo for example

Copy link
Collaborator Author

@Josh-Cena Josh-Cena May 27, 2022

Choose a reason for hiding this comment

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

My idea is that if we can namespace related things together it will be better in the long term? This will be used by plugin authors

Strong agree. I feel like we have too many things scattered at the root.

we'll likely want to make the per-locale dir configurable too later,

That's already included in the code as a TODO, and also why localizationDir includes the locale as a part. In the future, when we have i18n.locales.fr.path, we can simply use that to replace our current approach of using locale. Each locale's path will be relative to i18n.path, so we can still default to the locale name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But in the future I'd rather not ask plugin authors to have to use a long thing like i18n.localeConfigs[i18n.currentLocale].path, that would be simpler to just use i18n.localizationDir?

Also wonder if we shouldn't use plurals today. Does it make sense to have fr-FR + fr as localized dirs at the same time, and implement a fallback?

Copy link
Collaborator Author

@Josh-Cena Josh-Cena May 27, 2022

Choose a reason for hiding this comment

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

They would never use i18n.localeConfigs[i18n.currentLocale].path, though—i18n.localizationDir is already the root dir for the current locale, context-aware. I.e. if you are building for -l zh-Hans, the i18n.localizationDir is already <siteDir>/i18n/zh-Hans, combining i18n.localeConfigs[i18n.currentLocale].path and i18n.path, and plugins can directly read under that folder.

It's mentioned in the JSDoc of this API:

Constructed with i18n.path + i18n.currentLocale.path

The only use-case is like for the docs plugin's versioning CLI, where it needs to statically know the file location of every locale, instead of relying on the current locale. I think we can expose a util function for that.

packages/docusaurus-types/src/index.d.ts Outdated Show resolved Hide resolved
Josh-Cena and others added 2 commits May 27, 2022 19:48
# Conflicts:
#	packages/docusaurus/src/server/translations/__tests__/translations.test.ts
Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Ok fine 👍

Not 100% sure it's the best API we can find

But we'll be able to deprecate context fields with a warning in the future if we change our mind so let's move on

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Some questions remain:

  • How do we document this?
  • How do we teach this to plugin authors
  • How do we avoid annoying the existing community and plugin authors?

For reference, many existing packages/sites seem to use our util: getPluginI18nPath

https://github.com/search?q=getPluginI18nPath&type=code

We should find a way to keep it retro-compatible and emit a warning IMHO, or we create a brand new util function and just deprecate the old one

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Jun 2, 2022

many existing packages/sites seem to use our util: getPluginI18nPath

The API for that util has changed as well, so when they upgrade, they will know.

How do we avoid annoying the existing community and plugin authors?

It's not the first time this happened😅 It used to be a quite contrived and wonky API, and now with a special localizationDir it should be more stable into the future. Otherwise, we would have to change it again when we make the path customizable for each locale.

@slorber
Copy link
Collaborator

slorber commented Jun 2, 2022

The API for that util has changed as well, so when they upgrade, they will know.

This will break sites (and potentially plugins too), possible with a hard to understand error message, or fail silently to load translations.

We want users to notice and simplify the upgrade path

I'm not against this API change at all, but maybe we could just keep the old one too for retrocompatibility, and emit a clear warning?

TS overload, or the new API could have a different name

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Jun 2, 2022

I don't really want to think about backward compatibility in beta, particularly I don't really see any high-impact plugins in that search result. The very first result in that list, https://github.com/Computerization/computerization.github.io, is a site I maintain. Literally 90% of those results are from source code forked from us, and they are unlikely to upgrade anyway. I think the actual impact is quite low, if not none.

@slorber
Copy link
Collaborator

slorber commented Jun 2, 2022

yes agree, after looking deeper it seems fine and unlikely to be found in popular third-party plugins 👍

Let's merge then

We'll figure out how to doc/tech this later as there are other options to add anyway

@slorber slorber merged commit abe5450 into main Jun 2, 2022
@slorber slorber deleted the jc/i18n-path branch June 2, 2022 15:37
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.

3 participants