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

Ensure updated locales are auto-reloaded in electron renderer dev mode #4066

Merged
merged 3 commits into from
Oct 25, 2023
Merged

Ensure updated locales are auto-reloaded in electron renderer dev mode #4066

merged 3 commits into from
Oct 25, 2023

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Sep 24, 2023 •
edited
Loading

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

Ensure updated locales are auto-reloaded in electron renderer dev mode

Webpack does not watch locale files to rebuild
So added webpack-watch-external-files-plugin
Also updated internal locale processing plugin to only read changed files (mtime comparison)

Screenshots

Limited emit for locale JSON after en-US.yaml changed
image

No emit for locale JSON after renderer file changed
image

Testing

Basic

  • yarn && yarn dev
  • Switch to en-US
  • Update translation entry value (e.g. You have disabled automatic subscription fetching)
  • Confirm value update displayed on screen
  • (Optional) Repeat with another locale

Fix locales getting unnecessarily reprocessed for incremental builds

See #3893

Build

  • yarn && yarn pack:renderer
  • (Optional) yarn && yarn build

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

  • Only affects renderer not web one (different config file)
  • Does NOT watch activeLocales.json (it's rarely changed in dev)
  • Probably easier to use split view to review scripts/ProcessLocalesPlugin.js

@github-actions github-actions bot added PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review labels Sep 24, 2023
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 24, 2023 02:33
@PikachuEXE PikachuEXE force-pushed the dev/reload-modified-locale branch from 981f4e9 to e2f4665 Compare September 24, 2023 02:37
@absidue absidue disabled auto-merge September 24, 2023 08:53
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) September 24, 2023 08:53
@absidue
Copy link
Member

absidue commented Sep 24, 2023 •
edited
Loading

Didn't mean to click that.

I haven't tested this yet, but I don't think this change will do what you think it will, as FreeTube itself caches them too, it only loads each locale once. So you'll need changes inside FreeTube along with the webpack ones. https://github.com/FreeTubeApp/FreeTube/blob/development/src/renderer/i18n/index.js#L14-L18

Also have you tested a release build yarn pack:renderer, because at the moment you are adding the watch plugin regardless of build type. This is how a release only plugin is handled in the main process config: https://github.com/FreeTubeApp/FreeTube/blob/development/_scripts/webpack.main.config.js#L50

@PikachuEXE
Copy link
Collaborator Author

Watch external file plugin now only added in dev mode

But I have tested the behaviour with multiple locales and it's working for me
Please test first :)

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2023

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 Oct 2, 2023
@PikachuEXE PikachuEXE force-pushed the dev/reload-modified-locale branch from dd2beed to 07da6d0 Compare October 3, 2023 13:31
@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2023

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

@github-actions
Copy link
Contributor

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

@PikachuEXE PikachuEXE force-pushed the dev/reload-modified-locale branch from 07da6d0 to 1c833e2 Compare October 17, 2023 00:32
@github-actions
Copy link
Contributor

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

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 17, 2023
@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

Note to self: test after @absidue review is in

@absidue
Copy link
Member

absidue commented Oct 22, 2023

Okay, so it works and as I suspected, it does so by reloading the entire application, basically the same that would happen when you open a new window, so very expensive but at least it works.

Just remember not to change any strings while you are on the subscriptions page and have automatic subscription fetching enabled, otherwise you'll have fun with your screen filled with falling back to RSS toasts after a few edits.

Maybe in the future we can look into a more efficient solution but for now this works.

absidue
absidue requested changes Oct 22, 2023
View reviewed changes
_scripts/webpack.renderer.config.js Outdated Show resolved Hide resolved
_scripts/ProcessLocalesPlugin.js Outdated Show resolved Hide resolved
_scripts/ProcessLocalesPlugin.js Outdated Show resolved Hide resolved
_scripts/ProcessLocalesPlugin.js Outdated Show resolved Hide resolved
@PikachuEXE PikachuEXE requested a review from absidue October 23, 2023 06:29
@FreeTubeBot FreeTubeBot merged commit 342444f into FreeTubeApp:development Oct 25, 2023
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 25, 2023
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Oct 26, 2023
@PikachuEXE
Merge branch 'development' into feature/playlist-2023-05
7cd0aec 
* development: (26 commits)
  Translated using Weblate (Portuguese)
  Ensure updated locales are auto-reloaded in electron renderer dev mode (FreeTubeApp#4066)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Russian)
  Translated using Weblate (Latvian)
  Added translation using Weblate (Latvian)
  Translated using Weblate (Portuguese)
  Bump sass from 1.69.3 to 1.69.4 (FreeTubeApp#4216)
  Bump the stylelint group with 1 update (FreeTubeApp#4214)
  Bump the eslint group with 2 updates (FreeTubeApp#4213)
  Bump vue from 2.7.14 to 2.7.15 (FreeTubeApp#4217)
  Bump electron from 27.0.0 to 27.0.2 (FreeTubeApp#4215)
  Translated using Weblate (Portuguese)
  Add logo hover styling (FreeTubeApp#4209)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Portuguese)
  Update CONTRIBUTING.md (FreeTubeApp#4208)
  Translated using Weblate (Portuguese)
  Translated using Weblate (Samoan)
  ...
@PikachuEXE PikachuEXE deleted the dev/reload-modified-locale branch October 26, 2023 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChunkyProgrammer ChunkyProgrammer ChunkyProgrammer approved these changes

@efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc efb4f5ff-1298-471a-8973-3d47447115dc approved these changes

@absidue absidue absidue approved these changes

@kommunarr kommunarr Awaiting requested review from kommunarr

Assignees
No one assigned
Labels
PR: dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@PikachuEXE @absidue @efb4f5ff-1298-471a-8973-3d47447115dc @ChunkyProgrammer @FreeTubeBot