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

Add ScriptLoader #2417

Open
wants to merge 18 commits into
base: dev/1.4
Choose a base branch
from
Open

Add ScriptLoader #2417

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ed78593
feat: add script loader
MrKou47 Oct 31, 2024
2a1e40d
fix: lint error
MrKou47 Oct 31, 2024
0a214c7
chore: add test
MrKou47 Nov 1, 2024
9081f74
Update AssetType.ts
GuoLei1990 Nov 1, 2024
2ef9c8e
Update index.ts
GuoLei1990 Nov 1, 2024
e7bf294
chore: add ScriptLoader test
MrKou47 Nov 1, 2024
12357bb
chore: update Loader with clear context
MrKou47 Nov 1, 2024
f67960e
feat: let import() as it is
MrKou47 Nov 2, 2024
44025d7
chore: lint
MrKou47 Nov 2, 2024
89b957b
Update ScriptLoader.ts
GuoLei1990 Nov 3, 2024
fde6eca
chore: export ESModule type
MrKou47 Nov 5, 2024
e68253f
Update ScriptLoader.ts
GuoLei1990 Nov 5, 2024
80e4eed
Update ScriptLoader.ts
GuoLei1990 Nov 5, 2024
76d32a5
Update ScriptLoader.ts
GuoLei1990 Nov 5, 2024
fe5e35c
Update ScriptLoader.test.ts
GuoLei1990 Nov 5, 2024
ac334dc
Update ScriptLoader.ts
GuoLei1990 Nov 5, 2024
cd9d4bb
Update ScriptLoader.ts
GuoLei1990 Nov 5, 2024
a443c04
chore: update test
MrKou47 Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion packages/core/src/asset/AssetType.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,5 +56,7 @@ export enum AssetType {
/** Source Font, include ttf、 otf and woff. */
SourceFont = "SourceFont",
/** Project asset. */
Project = "project"
Project = "project",
/** Script in ES module */
Script = "Script"
}
20 changes: 20 additions & 0 deletions packages/loader/src/ScriptLoader.ts
View file
Open in desktop
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;
}
Copy link

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:

  1. Stricter typing instead of any
  2. JSDoc documentation explaining its purpose
  3. Validation for required properties
+/**
+ * Represents the structure of an ES module loaded by ScriptLoader.
+ * @property default - Optional default export containing the Script
+ * @property [key: string] - Additional named exports from the module
+ */
 interface ESModuleStructure {
   default?: Script;
-  [key: string]: any;
+  [key: string]: Script | unknown;
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
interface ESModuleStructure {
default?: Script;
[key: string]: any;
}
/**
* Represents the structure of an ES module loaded by ScriptLoader.
* @property default - Optional default export containing the Script
* @property [key: string] - Additional named exports from the module
*/
interface ESModuleStructure {
default?: Script;
[key: string]: Script | unknown;
}


@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(url) as Promise<ESModuleStructure>)
.then((esModule) => {
resolve(esModule);
})
.catch(reject);
});
}
}
2 changes: 2 additions & 0 deletions packages/loader/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@
import "./Texture2DLoader";
import "./TextureCubeLoader";
import "./ktx2/KTX2Loader";
import "./ScriptLoader";

export { GLTFLoader } from "./GLTFLoader";
export type { GLTFParams } from "./GLTFLoader";
export * from "./SceneLoader";
export type { Texture2DParams } from "./Texture2DLoader";
export { parseSingleKTX } from "./compressed-texture";
export { ScriptLoader } from './ScriptLoader'

Check failure on line 29 in packages/loader/src/index.ts

View workflow job for this annotation

GitHub Actions / lint 

Replace `'./ScriptLoader'` with `"./ScriptLoader";`
GuoLei1990 marked this conversation as resolved.
Show resolved Hide resolved
export * from "./gltf";
export { KTX2Loader, KTX2Transcoder } from "./ktx2/KTX2Loader";
export { KTX2TargetFormat } from "./ktx2/KTX2TargetFormat";
Expand Down
52 changes: 52 additions & 0 deletions tests/src/loader/ScriptLoader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { expect } from "chai";
import { WebGLEngine, Script } from '@galacean/engine'
import { AssetType } from "@galacean/engine-core";

let engine: WebGLEngine;

interface ESModuleStructure {
default?: Script;
[key: string]: any;
}

before(async () => {
const canvasDOM = document.createElement("canvas");
canvasDOM.width = 1024;
canvasDOM.height = 1024;
engine = await WebGLEngine.create({ canvas: canvasDOM });
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add test cleanup to prevent memory leaks.

The test setup creates a WebGLEngine instance but doesn't clean it up after tests complete. Consider adding an after hook.

Add this cleanup code:

+after(() => {
+  engine.destroy();
+});
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
before(async () => {
const canvasDOM = document.createElement("canvas");
canvasDOM.width = 1024;
canvasDOM.height = 1024;
engine = await WebGLEngine.create({ canvas: canvasDOM });
});
before(async () => {
const canvasDOM = document.createElement("canvas");
canvasDOM.width = 1024;
canvasDOM.height = 1024;
engine = await WebGLEngine.create({ canvas: canvasDOM });
});
after(() => {
engine.destroy();
});


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;
});
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve test reliability and security.

Several concerns with the current implementation:

  1. Using an external CDN in tests makes them dependent on network connectivity and third-party availability
  2. Missing error handling for failed requests
  3. No timeout specified for the async operation

Consider these improvements:

 it("loader from string url", async () => {
+  const timeout = 5000; // 5 seconds
+  try {
     engine.resourceManager.load<ESModuleStructure>({
       url: "https://cdn.jsdelivr.net/npm/[email protected]/+esm",
       type: AssetType.Script
     })
-    .then((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;
-    });
+    }, { timeout });
+  } catch (error) {
+    throw new Error(`Script loading failed: ${error.message}`);
+  }
 });

Consider mocking the external dependency for more reliable tests.

Committable suggestion skipped: line range outside the PR's diff.


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)
});
});
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add resource cleanup and improve test assertions.

The test creates a Blob URL but doesn't clean it up, which could lead to memory leaks. Also, the assertions could be more specific.

Apply these improvements:

 it("loader from blob raw script text", async () => {
   const esModuleString = `
     export const colord = "colord";
     export const getFormat = () => "getFormat";
     export default colord;
   `
+  const blobUrl = URL.createObjectURL(new Blob([esModuleString], { type: "application/javascript" }));
+  try {
     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.colord).to.equal("colord");
+      expect(script.getFormat).to.be.a("function");
+      expect(script.getFormat()).to.equal("getFormat");
       expect(script.default).equal(script.colord)
     });
+  } finally {
+    URL.revokeObjectURL(blobUrl);
+  }
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

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)
});
});
it("loader from blob raw script text", async () => {
const esModuleString = `
export const colord = "colord";
export const getFormat = () => "getFormat";
export default colord;
`
const blobUrl = URL.createObjectURL(new Blob([esModuleString], { type: "application/javascript" }));
try {
engine.resourceManager.load<ESModuleStructure>({
url: blobUrl,
type: AssetType.Script
})
.then((script) => {
expect(script).not.to.be.null;
expect(script.colord).to.equal("colord");
expect(script.getFormat).to.be.a("function");
expect(script.getFormat()).to.equal("getFormat");
expect(script.default).equal(script.colord)
});
} finally {
URL.revokeObjectURL(blobUrl);
}
});

});
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Based on the implementation review, I can now provide a final response:

Dynamic script loading requires additional security measures

The ScriptLoader uses the native ES module import() function to load scripts, which inherits the following security features:

  1. Scripts can only be loaded from valid URLs or Blob URLs
  2. Scripts are executed in strict mode by default
  3. Scripts inherit Content Security Policy (CSP) restrictions from the host page

However, the loader lacks:

  1. Script integrity validation (SRI)
  2. Origin/domain validation
  3. Documentation about security best practices

Recommendations:

  • Add support for Subresource Integrity (SRI) hash validation
  • Document security considerations in the API docs
  • Consider adding domain allowlist functionality
🔗 Analysis chain

Consider security implications of dynamic script loading.

The ScriptLoader allows loading and executing scripts from external sources, which could pose security risks if used with untrusted content. Consider:

  1. Adding content security policies
  2. Implementing script integrity validation
  3. Adding documentation about security best practices

Let's check if there are any security measures in the main implementation:

Let me search for the ScriptLoader implementation to better understand the security context:

Let me check the ScriptLoader implementation to understand how scripts are loaded:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for security-related code in ScriptLoader implementation
rg -i "integrity|security|policy|trust" --type ts

Length of output: 96

Script:

#!/bin/bash
# Find and examine ScriptLoader implementation
fd ScriptLoader.ts

Length of output: 56

Script:

#!/bin/bash
# Look for script loading related code
rg -A 10 "AssetType.Script" --type ts

Length of output: 2275

Script:

#!/bin/bash
# Search for any validation or security checks in resource loading
rg -A 5 "load.*\(" --type ts

Length of output: 157332

Script:

#!/bin/bash
# Get the ScriptLoader implementation
cat packages/loader/src/ScriptLoader.ts

Length of output: 637

🧰 Tools
🪛 eslint

[error] 52-52: Insert ⏎

(prettier/prettier)

Loading