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

i18n: Deprecate getI18n, dcnpgettext #9485

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
3 participants
@aduth
Member

aduth commented Aug 30, 2018

This pull request seeks to deprecate two methods of the i18n module: getI18n and dcnpgettext. This should be non-impacting to most consumers, as the other functions exposed by the module are sufficient for the majority of usage. The intent here is to limit the maintenance burden in surfacing internal implementation details, avoiding a commitment to maintaining interface parity with the underlying Jed tooling.

Testing instructions:

There should be no application impact in these changes.

Verify localization works as expected (site language respected in Gutenberg UI strings).

Verify the deprecated functions are still exposed on the public interface, but log a deprecated message in their usage:

image

@aduth aduth added this to In Progress in API freeze via automation Aug 30, 2018

@gziolo gziolo added this to the 3.8 milestone Sep 5, 2018

@gziolo

gziolo approved these changes Sep 5, 2018

It looks good. It still needs to be rebeased before we proceed.

Show outdated Hide outdated packages/i18n/src/test/index.js
@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

Realizing that my cleverness with withDeprecation might backfire with being detected by ESLint rules, as we've had issues before. I might "dumb it down" before merging.

Member

aduth commented Sep 5, 2018

Realizing that my cleverness with withDeprecation might backfire with being detected by ESLint rules, as we've had issues before. I might "dumb it down" before merging.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 5, 2018

Member

Realizing that my cleverness with withDeprecation might backfire with being detected by ESLint rules, as we've had issues before. I might "dumb it down" before merging.

Dumbed down in rebased 1f0c639.

Member

aduth commented Sep 5, 2018

Realizing that my cleverness with withDeprecation might backfire with being detected by ESLint rules, as we've had issues before. I might "dumb it down" before merging.

Dumbed down in rebased 1f0c639.

@aduth aduth merged commit ade2e1e into master Sep 5, 2018

2 checks passed

codecov/project No report found to compare against
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Sep 5, 2018

@aduth aduth deleted the deprecate/i18n-geti18n-dcnpgettext branch Sep 5, 2018

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 7, 2018

Member

FYI I'm seeing the deprecation logged with Yoast active. @omarreiss @atimmer Could you test to see whether the suggested alternatives could serve in place of your getI18n usage? For what reason are you using it?

Member

aduth commented Sep 7, 2018

FYI I'm seeing the deprecation logged with Yoast active. @omarreiss @atimmer Could you test to see whether the suggested alternatives could serve in place of your getI18n usage? For what reason are you using it?

@atimmer

This comment has been minimized.

Show comment
Hide comment
@atimmer

atimmer Sep 10, 2018

Member

@aduth We currently use it to check if our translations have already been loaded by setLocaleData. It's not required but was for extra safety, we can drop it I think.

We have an issue on Yoast SEO to track this: Yoast/wordpress-seo#10945.

Member

atimmer commented Sep 10, 2018

@aduth We currently use it to check if our translations have already been loaded by setLocaleData. It's not required but was for extra safety, we can drop it I think.

We have an issue on Yoast SEO to track this: Yoast/wordpress-seo#10945.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment