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

Remove localized global state #268

Merged
merged 6 commits into from
Apr 3, 2023
Merged

Conversation

arctic-hen7
Copy link
Member

Unfortunately, the current version of Perseus has a critical issue in its global state system: we support locale-dependent global state. This means you can generate a different global state for en-US, fr-FR, zh-CN, etc., which, although there are no immediately obvious use-cases, seemed like a good idea at the time. I have to apologize then for this PR, which removes this feature totally in favor of unlocalized global state, after #267 was reported, which shows that, not only does localized global state fail with the locale redirection system, in its current form, it is also incompatible with locale switching. This means that, if a user switches from en-US to fr-FR, the global state held in the browser will still be that for en-US. Adding state thawing to this issue only increases the complexity of a solution.

In light of all this, there are two solutions to the problem:

  1. Remove localized global state entirely in favor of an unlocalized version as was supported up to v0.4.0-beta.10, or
  2. Make global states be transmitted together with translations, and allow the user to specify an amalgamation function for when the user switches locales.

In my view, as the maintainer, option 2 is not only significantly more complex, but also more opaque, and substantially more difficult to communicate to new users: forcing anyone who uses global state and internationalization to define some magical function does not seem user-friendly to me at all.

Therefore, I have decided to implement option 1, which means the following:

  • Apps using global state without internationalization are totally unaffected
  • Apps using i18n without global state are totally unaffected
  • Apps using i18n with one global state for all locales are totally unaffected
  • Apps that were using locale-dependent global state will not be able to use this feature anymore, which may force significant refactoring
  • All apps will have to remove the locale parameter from their global state generation functions

The last one of these constitutes a breaking change for all users, while the second-last of these is by far the most impactful consequence of this change. To any users currently using locale-dependent global state, I can only sincerely apologize for this: I hate removing features, and, in general, I am careful to only implement features I am confident won't need to ever be rolled-back. In this case, I made a mistake. Again, I am sorry for this, but I think it's the best decision for the project as a whole in the longer-term (maintaining a version of Perseus that uses option 2, as outlined above, would, in my view, become extremely burdensome for future contributors). To replace locale-dependent global state, I would recommend the translations system, which, in extreme cases, or as a temporary patch mechanism, can be used to store structured data if absolutely necessary (since a custom TranslationsManager can be provided). Any users needing help with fixing their own systems after this change should join our Discord server, and we will be more than happy to help.

This PR's description is deliberately long to justify this change to any users adversely impacted by it (despite the fact that I am not aware of any usage of locale-dependent global state in the wild). I don't think this change will have a significant impact on the Perseus community at large, but, from personal experience with other projects, if there is even one user who was using locale-dependent global state, they deserve a proper explanation of why the rug is being yanked out underneath them.

This PR closes #267.

This completely axes the feature, and returns global state to a
one-per-app system.

BREAKING CHANGE: removed localized global state (global state functions
should no longer take a locale)
This fixes #267, but only when global state is unlocalized.
@ktsimpso
Copy link

Thanks for this. Though I do have one request as part of this. I currently use global state to store the current locale. It would be nice if there was a way to look that up. My other global state does not interact with the locale at all. That would solve the issue for my site. It's too bad you had to remove a feature to do it :(

@arctic-hen7
Copy link
Member Author

@ktsimpso that's already possible through the Reactor API. Use .get_translator().get_locale() in any page to get the current locale.

@ktsimpso
Copy link

@ktsimpso that's already possible through the Reactor API. Use .get_translator().get_locale() in any page to get the current locale.

Awesome, then this should 100% resolve any issues my app has.

Not strictly relevant to this PR, but important nonetheless. Note that
this includes a progression of the `minify-html-onepass` crate version,
which *could* cause problems, so that should be reverted to v0.10.1 if
there are issues.
@arctic-hen7 arctic-hen7 merged commit 2017f69 into main Apr 3, 2023
@arctic-hen7 arctic-hen7 deleted the feat-remove-localized-global-state branch April 3, 2023 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants