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

Add a polyfill for vue-i18n's useI18n() composable #6042

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

absidue
Copy link
Member

@absidue absidue commented Oct 30, 2024

Add a polyfill for vue-i18n's useI18n() composable

Pull Request Type

  • Refactoring - Composition API migration

Description

This pull request introduces a polyfill for vue-i18n 9+'s useI18n composable as well as a custom auto-fixable ESLint rule to enforce the use of the polyfill.

Why?

  • vue-i18n 8, the version of vue-i18n that is compatible with Vue 2, doesn't have the useI18n composable, so this polyfill allows us to already migrate components that need to use vue-i18n in the JavaScript section (e.g. accessing the current locale to pass to Intl APIs or using the translate function in computeds like we do for the drop down labels).
  • Even once we upgrade to Vue 3, vue-i18n 9+'s useI18n composable cannot be used while the vue-i18n instance is in legacy mode legacy: true, but we need legacy mode for components that are still using the Options API (unless we migrate every single component to the Composition API before we use Vue 3, which is unlikely to happen)
  • vue-i18n 9 does have an allowComposition option, however that option comes with some serious limitations (https://vue-i18n.intlify.dev/guide/migration/vue3#limitations) such as requiring the call to useI18n to be inside the nextTick() callback which is only fired after the component has already been mounted/rendered and was removed in vue-i18n 10 (https://vue-i18n.intlify.dev/guide/migration/breaking10.html#drop-allowcomposition-option). The polyfill in this PR doesn't have those limitations.
  • Compared to importing the vue-i18n instance and accessing the properties on it directly (what we currently do in the ft-shaka-video-player.js file), this provides a drop-in replacement for the useI18n function, so once we turn off legacy mode, all we need to do is remove the ESLint rule and update the imports to point to vue-i18n instead.
  • I created an auto-fixable ESLint rule so you don't even have to think about which useI18n function you import or that your editor has imported the wrong one, as the ESLint rule will catch it and can fix the import for you.
  • The only extra work this introduces for the initial Vue 3 migration is a few minor changes to the polyfill file, all files that import the polyfill should "just work".
  • The vue-i18n ESLint rules work correctly with this polyfill.

Screenshots

eslint-error

eslint-quick-fix

using-polyfill

vue-i18n-eslint

Testing

Testing the polyfill

  1. Open two FreeTube windows
  2. In the first window navigate to a video
  3. In the second window open the general settings
  4. Change the display locale and check that the video player updates it's language (e.g. hover over the playback rate selector)
  5. Take a screenshot and make sure the toast message is translated

Testing the ESLint rule

  1. Try adding import { useI18n } from 'vue-i18n' to a file and check that ESLint detects it and that the fix works (it should change the text after the from part).
  2. Try adding import { useI18n, createI18n } from 'vue-i18n' to a file and check that ESLint detects and and that the fix works (it should edit the existing import to remove the useI18n part and create a second import importing the useI18n polyfill)

Desktop

  • OS: Windows 10
  • OS Version: 10
  • FreeTube version: d678863

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 30, 2024 15:18
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 30, 2024
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

image

Before fix (yarn run eslint-lint-fix)
image
After fix
image

eslint.config.mjs Show resolved Hide resolved
PikachuEXE
PikachuEXE previously approved these changes Nov 1, 2024
Copy link
Contributor

github-actions bot commented Nov 2, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added PR: merge conflicts / rebase needed and removed PR: waiting for review For PRs that are complete, tested, and ready for review labels Nov 2, 2024
@efb4f5ff-1298-471a-8973-3d47447115dc

rip didnt see that the conflicts label got applied

Copy link
Contributor

github-actions bot commented Nov 3, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@absidue absidue added the PR: waiting for review For PRs that are complete, tested, and ready for review label Nov 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants