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

Set process.platform at build time #6784

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

absidue
Copy link
Member

@absidue absidue commented Feb 10, 2025

Set process.platform at build time

Pull Request Type

  • Performance improvement

Description

For a while now I've been wondering what the best approach would be for detecting the OS that FreeTube is running on for OS specific functionality like keyboard shortcuts when we disable nodeIntegration. Currently we use Node.js' process.platform property. Originally I was planning to try and detect it with navigator.platform and navigator.userAgentData like shaka-player does, but then after looking at native code in a different project I remembed that compile to native languages do the OS specific stuff at build time and there is no reason why we can't do the same, we already have various other build flags.

The webpack config for the renderer and main files use the process.platform value of the machine that the build is running on and the web config uses undefined which is the same as the current behaviour as the process polyfill doesn't support the platform property. This shouldn't cause any problems as the _scripts/build.js script that calls electron-builder for the production builds already uses the OS of the machine it is running on and during development we launch electron on the same machine that webpack is running on so it won't cause issues there either. The only case it could cause problems is if someone were to run yarn run pack on one machine, copy the output to another machine running a different OS and run yarn run build-release there, but nobody should be doing that.

We can't get rid of the process polyfill just yet as the util polyfill that the web version of nedb uses, uses the process global, I am working on a modernised fork of nedb that should resolve that though (switch to ESM, remove the callback APIs and deprecated functionality, the idea is that we will only have to change the package name in the package.json file in FreeTube without needing any code changes).

All numbers are in bytes. For the platform specific after numbers, I hardcoded the process.platform to specific platforms in the webpack config and ran yarn run clean && yarn run pack.

Windows macOS Linux
main.js before 118813 118813 118813
main.js after 117235 (-1578) 118394 (-419) 117204 (-1609)
renderer.js before 2604678 2604678 2604678
renderer.js after 2603626 (-1052) 2603464 (-1214) 2603545 (-1133)

Testing

yarn run dev and check that platform specific things like keyboard shortcuts still work.

Desktop

  • OS: Windows
  • OS Version: 10
  • FreeTube version: 83d99dd

@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Feb 10, 2025
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) February 10, 2025 21:51
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.

tested macOS keyboard shortcut in dev (Go back/forward one page

@FreeTubeBot FreeTubeBot merged commit 3261e36 into FreeTubeApp:development Feb 17, 2025
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 Feb 17, 2025
@absidue absidue deleted the build-time-platform branch February 17, 2025 21:33
PikachuEXE added a commit to PikachuEXE/FreeTube that referenced this pull request Feb 20, 2025
* development: (58 commits)
  Convert FtSubscribeButton and watch-video-info SCSS to CSS (FreeTubeApp#6814)
  Use numbers instead of strings for the DBActions and SyncEvents constants (FreeTubeApp#6815)
  Translated using Weblate (English (United Kingdom))
  Fix: search history text overflows if search term is long (FreeTubeApp#6728)
  Check if a keyboard composition session is active when pressing 'Enter' on ft-input (FreeTubeApp#6799)
  use hq img (FreeTubeApp#6826)
  Move the choose default folder logic to the main process (FreeTubeApp#6811)
  Translated using Weblate (French)
  Translated using Weblate (Turkish)
  Set process.platform at build time (FreeTubeApp#6784)
  Use logical spec for float (FreeTubeApp#6783)
  Migrate DataSettings to the composition API (FreeTubeApp#6785)
  Local API: Improve audio quality by sorting streams, highest bitrate first (FreeTubeApp#6807)
  Bump sass from 1.84.0 to 1.85.0 (FreeTubeApp#6825)
  Bump webpack from 5.97.1 to 5.98.0 (FreeTubeApp#6820)
  Bump postcss from 8.5.1 to 8.5.2 in the stylelint group (FreeTubeApp#6819)
  Bump the babel group with 2 updates (FreeTubeApp#6817)
  Bump globals from 15.14.0 to 15.15.0 (FreeTubeApp#6823)
  Bump sass-loader from 16.0.4 to 16.0.5 (FreeTubeApp#6822)
  Bump eslint from 9.20.0 to 9.20.1 in the eslint group (FreeTubeApp#6818)
  ...

# Conflicts:
#	src/renderer/components/watch-video-info/watch-video-info.scss
#	src/renderer/components/watch-video-info/watch-video-info.vue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants