ApiFetch: Remove i18n package dependency #62934
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sirreal
added
the
[Type] Task
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -15 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 2ea5d78. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9700331609
|
Why not only return a code then, if they are not user facing? These codes could be documented, and "translated" into readable text by the implementor of api-fetch. |
sirreal
commented
Jun 27, 2024
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Enhancements | |||
There was a problem hiding this comment.
This is probably a breaking change.
Suggested change
| ### Enhancements | |
| ### Breaking Changes |
This was a bad assumption on my part. These do look like strings that are intended to be printed to the end users:
It seems very likely these are printed in toast messages somewhere. |
|
@sirreal Yes, but my point remains: if you really want to remove translations from this package, you could leave the code only, and then handle the code in the consuming package? |
|
(And maybe just leave an untranslated string for back compat, not sure) |
|
Moving from a translated string to an untranslated string is a regression. There's a tradeoff that has some benefits and I believe the tradeoff is justified. Moving from a translated string to a code is a bigger regression with little benefit. The error messages are mostly guesses as to what happened, but removing them entirely doesn't seem to add additional benefit over providing an untranslated string. You have given me an idea that may allow us to avoid the regression entirely while unblocking work on an api-fetch module. We can move the error messages to their own file and only include them in the apiFetch script form. We can still consider this PR and removing the dependency, but may not block the modules work. |
|
The messages are quite simple so you could move them to the config file and expose the translated form from the server using the new way to share data with the client. It will have the expected impact on the frontend regarding the bundle size. In the editor it won’t have any negative impact other than a bit indirect way to handle these translations. The description of PR needs to get updated to reflect the fact that these messages are user facing in the editor. |
gziolo
added
[Type] Performance
|
I like that idea @gziolo. I'll close this PR because we have better alternatives. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Remove
api-fetchpackage's dependency oni18n.Why?
The primary reason to remove this is to unblock work on an api-fetch module: https://core.trac.wordpress.org/ticket/60647
This package is for making api calls and there's not a strong motivation to localize most strings. None of the localized texts are end-user-facing. This dependency was only used to localize error messages targeted at developers. Other packages, for example hooks, do not localize these types of messages:
gutenberg/packages/hooks/src/validateHookName.js
Line 19 in fe67010
Every dependency may increase the amount of JavaScript required on the client, so removing a dependency like this may improve the performance of sites using
apiFetch.How?
Remove the i18n dependency and use static strings instead of localized ones.
Testing Instructions
CI passes.