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.

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

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

Merged
merged 4 commits into from
Feb 20, 2023
Merged

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

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 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

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

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
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.

@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@susnux
Drop OC.addTranslations deprecated since NC17 and not used by any N…
7e3372c 
…C app

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@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
Deprecate OC.L10N functions in favor of translation functions from `@…
cdc3848 
…nextcloud/l10n`

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@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 <rpm@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Feb 20, 2023

/compile

@nextcloud-command
Compile assets
bfad19c 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@susnux
Copy link
Contributor Author

susnux commented Feb 20, 2023

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

@juliushaertl juliushaertl merged commit 21f9688 into master Feb 20, 2023
@juliushaertl juliushaertl 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 l10n and translations
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
7 participants
@susnux @ChristophWurst @skjnldsv @Pytal @juliushaertl @szaimen @nextcloud-command