-
-
Notifications
You must be signed in to change notification settings - Fork 309
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 ScriptLoader
#2417
base: dev/1.4
Are you sure you want to change the base?
Add ScriptLoader
#2417
Changes from 8 commits
ed78593
2a1e40d
0a214c7
9081f74
2ef9c8e
e7bf294
12357bb
f67960e
44025d7
89b957b
fde6eca
e68253f
80e4eed
76d32a5
fe5e35c
ac334dc
cd9d4bb
a443c04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { AssetPromise, AssetType, Loader, LoadItem, resourceLoader, Script } from "@galacean/engine-core"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
interface ESModuleStructure { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
default?: Script; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[key: string]: any; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
@resourceLoader(AssetType.Script, ["js", "mjs"], false) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export class ScriptLoader extends Loader<ESModuleStructure> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
load(item: LoadItem): AssetPromise<ESModuleStructure> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return new AssetPromise((resolve, reject) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { url } = item; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
(import(/* @vite-ignore */url) as Promise<ESModuleStructure>) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.then((esModule) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
resolve(esModule); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.catch(reject); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider adding resource cleanup. The loader should implement cleanup logic to handle cases where the component is destroyed while scripts are still loading. This prevents memory leaks and ensures proper resource management. @resourceLoader(AssetType.Script, ["js", "mjs"], false)
export class ScriptLoader extends Loader<ESModuleStructure> {
+ private loadingScripts = new Set<string>();
+
load(item: LoadItem): AssetPromise<ESModuleStructure> {
return new AssetPromise((resolve, reject) => {
const { url } = item;
+ this.loadingScripts.add(url);
(import(/* @vite-ignore */ url) as Promise<ESModuleStructure>)
.then((esModule) => {
+ this.loadingScripts.delete(url);
resolve(esModule);
})
- .catch(reject);
+ .catch((error) => {
+ this.loadingScripts.delete(url);
+ reject(error);
+ });
});
}
+
+ destroy(): void {
+ this.loadingScripts.clear();
+ super.destroy?.();
+ }
} 📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: lint[failure] 13-13: |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,59 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { WebGLEngine } from "@galacean/engine-rhi-webgl"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { AssetType, Script } from "@galacean/engine-core"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import "@galacean/engine-loader"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { describe, it, expect, beforeAll, afterAll } from "vitest"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
let engine: WebGLEngine; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
interface ESModuleStructure { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
default?: Script; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
[key: string]: any; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
beforeAll(async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const canvasDOM = document.createElement("canvas"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
canvasDOM.width = 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
canvasDOM.height = 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
engine = await WebGLEngine.create({ canvas: canvasDOM }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
describe("ScriptLoader test", function () { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
it("loader from string url", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
engine.resourceManager.load<ESModuleStructure>({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
url: "https://cdn.jsdelivr.net/npm/[email protected]/+esm", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type: AssetType.Script | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.then((script) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.default).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.colord).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.getFormat).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.random).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve async test handling. The test uses async/await syntax but then switches to .then() chain. Consider using consistent async/await pattern and add proper error handling. it("loader from string url", async () => {
-
- engine.resourceManager.load<ESModuleStructure>({
+ const script = await engine.resourceManager.load<ESModuleStructure>({
url: "https://cdn.jsdelivr.net/npm/[email protected]/+esm",
type: AssetType.Script
- })
- .then((script) => {
- expect(script).not.to.be.null;
- expect(script.default).not.to.be.null;
- expect(script.colord).not.to.be.null;
- expect(script.getFormat).not.to.be.null;
- expect(script.random).not.to.be.null;
});
+ expect(script).not.to.be.null;
+ expect(script.default).not.to.be.null;
+ expect(script.colord).not.to.be.null;
+ expect(script.getFormat).not.to.be.null;
+ expect(script.random).not.to.be.null;
}); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
it("loader from blob raw script text", async () => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const esModuleString = ` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const colord = "colord"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const getFormat = () => "getFormat"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export default colord; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
engine.resourceManager.load<ESModuleStructure>({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
url: URL.createObjectURL(new Blob([esModuleString], { type: "application/javascript" })), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
type: AssetType.Script | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
.then((script) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.colord).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.getFormat).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.default).not.to.be.null; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
expect(script.default).equal(script.colord) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve test structure and async handling. Similar to the previous test, consider using consistent async/await pattern and structured assertions. it("loader from blob raw script text", async () => {
const esModuleString = `
export const colord = "colord";
export const getFormat = () => "getFormat";
export default colord;
`
- engine.resourceManager.load<ESModuleStructure>({
+ const blobUrl = URL.createObjectURL(
+ new Blob([esModuleString], { type: "application/javascript" })
+ );
+ try {
+ const script = await engine.resourceManager.load<ESModuleStructure>({
- url: URL.createObjectURL(new Blob([esModuleString], { type: "application/javascript" })),
+ url: blobUrl,
type: AssetType.Script
- })
- .then((script) => {
- expect(script).not.to.be.null;
- expect(script.colord).not.to.be.null;
- expect(script.getFormat).not.to.be.null;
- expect(script.default).not.to.be.null;
- expect(script.default).equal(script.colord)
- })
+ });
+ expect(script).not.to.be.null;
+ expect(script.colord).not.to.be.null;
+ expect(script.getFormat).not.to.be.null;
+ expect(script.default).not.to.be.null;
+ expect(script.default).equal(script.colord);
+ } finally {
+ URL.revokeObjectURL(blobUrl);
+ }
}); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
afterAll(function () { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
engine.resourceManager.gc(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
engine.destroy(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); |
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.
🛠️ Refactor suggestion
Enhance type safety and documentation for ESModuleStructure interface
The interface could benefit from:
any
📝 Committable suggestion