Skip to content

Deprecate and cleanup OC.L10N in favor of @nextcloud/l10n #36287

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.

Sign up for GitHub

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

Jump to bottom
Merged
juliusknorr merged 4 commits into from
Feb 20, 2023
Merged

Deprecate and cleanup OC.L10N in favor of @nextcloud/l10n #36287

juliusknorr merged 4 commits into from
Feb 20, 2023

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Jan 22, 2023 •
edited
Loading

Summary

This PR consists of three parts:

  1. Cleanup long standing deprecation from OC: OC.addTranslations was deprecated with NC17 and is not used anymore by any app within the github nextcloud organization
  2. Deprecate OC.getLocale and OC.getLanguage as they are provided by @nextcloud/l10n and drop duplicated code
  3. Deprecate and replace the translation functions implementation with the @nextcloud/l10n.

Checklist

@susnux susnux added enhancement 3. to review Waiting for reviews feature: language/translations (l10n/i18n) Localization and translation matters labels Jan 22, 2023
@susnux susnux mentioned this pull request Jan 22, 2023
Merged
@szaimen szaimen added this to the Nextcloud 26 milestone Jan 22, 2023
ChristophWurst
ChristophWurst approved these changes Jan 23, 2023
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

🧹

skjnldsv
skjnldsv reviewed Jan 24, 2023
View reviewed changes
core/src/OC/l10n.js Outdated
Comment on lines 71 to 73
* @deprecated 26.0.0 use `_unregister` from https://www.npmjs.com/package/@nextcloud/l10n
*/
_unregister: unregisterAppTranslations,
_unregister,
Copy link
Member

@skjnldsv skjnldsv Jan 24, 2023

Choose a reason for hiding this comment

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

is it a private method? Then it should not be exposed or it should be made public?

Copy link
Contributor Author

@susnux susnux Jan 24, 2023

Choose a reason for hiding this comment

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

This is currently only used in CI tests, but I do not see a reason why this function should not be on the public API.
Maybe we should change this on the @nextcloud/l10n package.

But for this PR I wanted to keep the changes small and do not introduce a breaking API change.

@susnux susnux force-pushed the feat/switch-pkg-l10n branch from a9a2799 to 1705765 Compare January 24, 2023 18:42
@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2023

/compile

@susnux
Copy link
Contributor Author

susnux commented Jan 24, 2023

Rebased onto current master

@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish 2. developing Work in progress blocked and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Jan 24, 2023
@susnux
Copy link
Contributor Author

susnux commented Jan 26, 2023

Blocked by nextcloud-libraries/nextcloud-l10n#571 (and we need to wait for a new release)
There was an issue introduced in the standalone implementation which caused the function to synchronously load translations instead of asynchronously, this is the reason the test fail here.

@susnux susnux force-pushed the feat/switch-pkg-l10n branch from 2437eba to 5a11c08 Compare January 26, 2023 17:16
@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@susnux susnux force-pushed the feat/switch-pkg-l10n branch from 5a11c08 to ffeabde Compare February 20, 2023 11:47
@susnux susnux added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress blocked labels Feb 20, 2023
@susnux
Copy link
Contributor Author

susnux commented Feb 20, 2023

/compile

@susnux
fix(jsunit): Modify OC.L10N tests as it does not depend on OC. an…
c97137e 
…ymore

Meaning we can not stub `getLocale` but must override the document attribute instead.

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux force-pushed the feat/switch-pkg-l10n branch from 23fea27 to c97137e Compare February 20, 2023 14:08
@susnux
Copy link
Contributor Author

susnux commented Feb 20, 2023

/compile

@nextcloud-command
Compile assets
bfad19c 
Signed-off-by: nextcloud-command <[email protected]>
@susnux
Copy link
Contributor Author

susnux commented Feb 20, 2023

The drone CI failure on the mysql DB tests seem unrelated.

@juliusknorr juliusknorr merged commit 21f9688 into master Feb 20, 2023
@juliusknorr juliusknorr deleted the feat/switch-pkg-l10n branch February 20, 2023 17:59
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@skjnldsv skjnldsv skjnldsv approved these changes

@Pytal Pytal Pytal approved these changes

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement feature: language/translations (l10n/i18n) Localization and translation matters

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

8 participants

@susnux @ChristophWurst @skjnldsv @Pytal @juliusknorr @szaimen @nextcloud-command