-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Use AssetListLoader to load assets in AppBase.preload #8177
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the AppBase.preload method to use the AssetListLoader utility for loading preload-marked assets, replacing the previous manual asset loading and event handling implementation. It also adds filtering to exclude already-loaded assets before starting the preload process, and includes comprehensive tests for the preload functionality.
Key changes:
- Replaced manual asset loading loop with AssetListLoader for cleaner code
- Added filter to exclude already-loaded assets from the preload process
- Added test coverage for preload functionality with both positive and negative test cases
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/framework/app-base.js | Refactored preload method to use AssetListLoader instead of manual asset loading; added filtering for already-loaded assets |
| test/framework/application.test.mjs | Added comprehensive tests for preload functionality covering both assets with preload=true and preload=false scenarios |
Comments suppressed due to low confidence (1)
src/framework/app-base.js:711
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
const assets = this.assets.filter(asset => asset.preload === true && asset.loaded === false)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| const assetListLoader = new AssetListLoader(assets, this.assets); | ||
|
|
||
| assetListLoader.on('progress', onAssetLoad); |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AssetListLoader only fires 'progress' events on successful asset loads, not on errors. This changes the behavior from the previous implementation where preload:progress was fired for both successful loads and errors. This means if some assets fail to load, the progress will not reach 1.0 (100%). Consider also handling the 'error' event from AssetListLoader, or document this behavior change.
| assetListLoader.on('progress', onAssetLoad); | |
| assetListLoader.on('progress', onAssetLoad); | |
| assetListLoader.on('error', onAssetLoad); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There exists no error event for AssetListLoader. We would need to make adjustments to the AssetListLoader class to make this work.
Co-authored-by: Copilot <[email protected]>
Description
Uses AssetListLoader in AppBase.preload to load assets marked as preload.
I also changed the fetching of assets to filter by loaded as well. By the time preload is called some assets might already be loaded. (Wasm modules, as well as some assets which are already loading because they were added to the asset registry. See #3107)
Also added tests for preload
Fixes #4185
Checklist