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

Fix /share and cleanup and reorganize frontend locale loading #25240

Merged
merged 3 commits into from Jun 2, 2023

Conversation

renchap
Copy link
Sponsor Member

@renchap renchap commented Jun 1, 2023

My react-intl upgrade PR broke the locale loading on /share. This was caused by getLocale being called when importing files, as it is called in the root scope of the file.

This is a bad practice and can easily lead to those files being imported/loaded while the locale is not loaded yet. The current code needs to dynamically import the various React components after loadLocale has been called, which is error-prone.

Now, locale data is loaded when the component/code is called, not imported. This avoid the need to dynamically import this code once the locale data has been loaded

While I was working on this, I made some additional changes that I did not wanted to do in my initial PR to not make it too complex:

  • Removed some unused files I missed in my last commit
  • No more locale loading as a side-effect
  • No need for the locale prop anymore for root React components, this is read from <html lang>
  • Converted locale-related files to Typescript
  • locale-related TS code is in mastodon/locales/, which only exports the needed functions & types

@renchap
Copy link
Sponsor Member Author

renchap commented Jun 1, 2023

Note: I can also rework this to not move the locale JSON files and have the TS code in another directory if you prefer (mastodon/intl?)

.prettierignore Outdated Show resolved Hide resolved
@ClearlyClaire
Copy link
Contributor

I understand the reason for moving the JSON files and I agree with it, but I'm not sure how well Crowdin handles that, so I'd like that to be postponed until we make sure this won't break anything with Crowdin.

@renchap
Copy link
Sponsor Member Author

renchap commented Jun 2, 2023

I understand the reason for moving the JSON files and I agree with it, but I'm not sure how well Crowdin handles that, so I'd like that to be postponed until we make sure this won't break anything with Crowdin.

I can leave the JSON in locales/ and move the TS files elsewhere, but I am not sure where. Ideas:

  • mastodon/intl/
  • mastodon/locale/ (without an s)
  • mastodon/utils/intl/

@ClearlyClaire
Copy link
Contributor

Do the TS files need to be kept out of that folder as a first step? If not, I'd postpone that split to a later PR, if yes, I'm fine with mastodon/intl

- No need for the `locale` prop anymore for root React components, this is read from `<html lang>`
- Converted locale-related files to Typescript
- No more locale loading as a side-effect. Locale data is loaded when the component/code is called, not imported. This avoid the need to dynamically import this code once the locale data has been loaded
`index.ts` re-exports what can be consumed by the app and nothing else
@renchap renchap changed the title Cleanup and reorganize frontend I18n files Cleanup and reorganize frontend locale loading Jun 2, 2023
@renchap
Copy link
Sponsor Member Author

renchap commented Jun 2, 2023

I removed the JSON file renaming.

All TS files are in mastodon/locales/ with the JSON files in the same directory. I split them in multiple files, index.ts re-exports what should be used in the app.

@ClearlyClaire ClearlyClaire changed the title Cleanup and reorganize frontend locale loading Fix /share and cleanup and reorganize frontend locale loading Jun 2, 2023
@ClearlyClaire ClearlyClaire added the bug Something isn't working label Jun 2, 2023
@ClearlyClaire ClearlyClaire merged commit b0780cf into mastodon:main Jun 2, 2023
32 checks passed
@renchap renchap deleted the cleanup-intl branch June 2, 2023 13:08
supernovae pushed a commit to Universeodon-com/mastodon that referenced this pull request Jun 7, 2023
skerit pushed a commit to 11ways/mastodon that referenced this pull request Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants