Skip to content

feat: Use DIR_ASSETS path to locate resource bundles #47439

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
Merged
merged 7 commits into from
Aug 4, 2025
Merged

feat: Use DIR_ASSETS path to locate resource bundles #47439

merged 7 commits into from
Aug 4, 2025

Conversation

itsananderson
Copy link
Member

@itsananderson itsananderson commented Jun 10, 2025 •
edited
Loading

Description of Change

Electron currently uses the DIR_MODULE constants DIR_EXE when querying the PathService to calculate resource paths, but the Chromium source and https://issues.chromium.org/issues/40203062 both discourage using these constants to locate assets, and instead recommend using DIR_ASSETS.

By default DIR_ASSETS is an alias for DIR_MODULE, but by updating Electron to use DIR_ASSETS, we align better with Chromium recommendations, and make it possible for apps with custom layouts to only need to override DIR_ASSETS.

In addition to changing the constants used internally for loading resources.pak and for the process.resourcesPath, this PR adds "assets" as a key that can be queried via app.getPath.

Because DIR_ASSETS is treated as an alias for DIR_MODULE by default, these changes should be a no-op for any existing apps.

Checklist

Release Notes

Notes: Internally switched to using DIR_ASSETS instead of DIR_MODULE/DIR_EXE to locate assets and resources, and added "assets" as a key that can be queried via app.getPath.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jun 10, 2025
itsananderson
itsananderson commented Jun 10, 2025
View reviewed changes
docs/api/app.md
* `temp` Temporary directory.
* `exe` The current executable file.
* `module` The `libchromiumcontent` library.
* `module` The location of the Chromium module. By default this is synonymous with `exe`.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is only tangentially related to the other changes, but libchromiumcontent hasn't existed for a long time, so it seemed worth updating these docs while I'm here.

@deepak1556
Copy link
Member

This looks good, can you make the following additional changes

  1. Use asset path for

    electron/shell/common/asar/archive_win.cc

    Lines 29 to 37 in d7ef8b7

    base::FilePath exe_path;
    if (!base::PathService::Get(base::FILE_EXE, &exe_path)) {
    LOG(FATAL) << "Couldn't get exe file path";
    }
    base::FilePath relative_path;
    if (!exe_path.DirName().AppendRelativePath(path_, &relative_path)) {
    return std::nullopt;
    }
  2. Add a unit test that default app.getPath('assets') matches the path where the exe is located path.dirname(app.getPath('exe')) to confirm no-op of this change.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jun 17, 2025
@itsananderson
Copy link
Member Author

Add a unit test that default app.getPath('assets') matches the path where the exe is located path.dirname(app.getPath('exe')) to confirm no-op of this change.

@deepak1556 I started working on this but then got pulled into other work for awhile.

What I've found by adding some tests is that app.getPath('assets') on Mac returns Electron.app/Contents/Frameworks/Electron Framework.framework/Resources
rather than Electron.app/Contents/Resources. This might make it tricky to reconcile the Mac asset paths without breaking changes, because I believe Chromium looks for resources under Electron.app/Contents/Frameworks/Electron Framework.framework/Resources while Electron looks for resources under Electron.app/Contents/Resources. Fixing these to be consistent would probably require updates to Electron app packagers at the very least, but if any apps are hardcoding resource lookups, they could still be broken if we change resource locations.

@deepak1556
Copy link
Member

deepak1556 commented Jun 20, 2025 •
edited
Loading

Thanks for looking into the tests, if it would make things easier, we can expose the assets key to just windows and linux in this PR. MacOS can be follow-up if there is interest in changing asset dirs for this platform.

My initial comment for tests was also specific to windows and linux since the code paths touched in this PR are specific to those platforms.

@deepak1556 deepak1556 added semver/minor backwards-compatible functionality api-review/requested 🗳 target/37-x-y PR should also be added to the "37-x-y" branch. target/38-x-y PR should also be added to the "38-x-y" branch. labels Jul 4, 2025
samuelmaddock
samuelmaddock reviewed Jul 17, 2025
View reviewed changes
Copy link
Member

@samuelmaddock samuelmaddock left a comment

Choose a reason for hiding this comment

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

API LGTM

@deepak1556
Copy link
Member

Pending one more API approval @electron/wg-api

codebytere
codebytere approved these changes Aug 1, 2025
View reviewed changes
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

API LGTM

@deepak1556 deepak1556 force-pushed the itsananderson/use-dir-assets-for-paths branch from 404b4d2 to 1596059 Compare August 2, 2025 01:03
@deepak1556
Copy link
Member

Failing tests are unrelated, merging.

@deepak1556 deepak1556 merged commit e7683bd into electron:main Aug 4, 2025
55 of 56 checks passed
@release-clerk
Copy link

release-clerk bot commented Aug 4, 2025

Release Notes Persisted

Internally switched to using DIR_ASSETS instead of DIR_MODULE/DIR_EXE to locate assets and resources, and added "assets" as a key that can be queried via app.getPath.

This was referenced Aug 4, 2025
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Aug 4, 2025

I have automatically backported this PR to "38-x-y", please check out #47950

@trop trop bot added in-flight/38-x-y and removed target/38-x-y PR should also be added to the "38-x-y" branch. labels Aug 4, 2025
@trop
Copy link
Contributor

trop bot commented Aug 4, 2025

I have automatically backported this PR to "37-x-y", please check out #47951

@trop trop bot added in-flight/37-x-y merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. and removed target/37-x-y PR should also be added to the "37-x-y" branch. in-flight/37-x-y in-flight/38-x-y labels Aug 4, 2025
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@itsananderson @kigh-ota
feat: Use DIR_ASSETS path to locate resource bundles (electron#47439)
fe09a3e 
* feat: Use DIR_ASSETS path to locate resource bundles

* Use DIR_ASSETS for calculating ASAR relative paths

* Add test to verify 'assets' matches parent dir of 'exe'

* Add Mac-specific test for assets path (but it is failing)

* test: Update app.getPath('assets') to expect an exception on Mac

* docs: Update docs for 'assets' path to indicate that it's only available on Windows + Linux

* fix: Don't define 'assets' mapping on macOS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@samuelmaddock samuelmaddock samuelmaddock left review comments

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

No one assigned

Labels

api-review/approved ✅ merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. semver/minor backwards-compatible functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@itsananderson @deepak1556 @samuelmaddock @codebytere