Add ScriptLoader
#2417
Add ScriptLoader
#2417
Conversation
WalkthroughThe changes in this pull request introduce a new enumeration value, Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Consider whether or not to use dynamic-import-polyfill 
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2417 +/- ##
==========================================
- Coverage 69.50% 69.47% -0.04%
==========================================
Files 524 522 -2
Lines 27405 27365 -40
Branches 4100 4097 -3
==========================================
- Hits 19049 19011 -38
- Misses 6852 6853 +1
+ Partials 1504 1501 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/ScriptLoader.ts (1)
3-3: Document the purpose of the decorator's boolean parameterThe
falseparameter in the decorator needs documentation to explain its purpose for future maintainability.Add a comment explaining the boolean parameter:
+ // The false parameter indicates... [please specify the purpose] @resourceLoader(AssetType.Script, ["js", "mjs"], false)packages/core/src/asset/AssetType.ts (2)
60-61: Consider enhancing the documentation for Script type.The current inline comment could be more descriptive about the supported module formats and usage.
Consider updating to match the documentation style of other entries:
- /** Script in ES module */ + /** + * ES module script. + * @remarks Supports .js and .mjs module files for dynamic loading. + */
59-59: Consider standardizing the casing of enum values.The
projectvalue uses lowercase while all other enum values (including the newScript) use PascalCase. This inconsistency could be confusing for developers.Consider updating to maintain consistency:
- Project = "project", + Project = "Project",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/core/src/asset/AssetType.ts(1 hunks)packages/loader/src/ScriptLoader.ts(1 hunks)
🔇 Additional comments (2)
packages/loader/src/ScriptLoader.ts (1)
8-8: Verify URL type safety
The direct casting of the import promise to Promise<Script> might hide type incompatibilities.
Let's verify the Script type definition and usage:
packages/core/src/asset/AssetType.ts (1)
59-61: LGTM! The new Script enum value is well-placed.
The addition of the Script type aligns with the PR's objective of supporting script loading functionality.
packages/loader/src/ScriptLoader.ts
Outdated
| import { AssetPromise, AssetType, Loader, LoadItem, resourceLoader, Script } from "@galacean/engine-core"; | ||
|
|
||
| @resourceLoader(AssetType.Script, ["js", "mjs"], false) |
There was a problem hiding this comment.
Consider adding input validation for script URLs
The loader handles both .js and .mjs files, but there's no URL validation before dynamic importing. This could pose security risks if untrusted URLs are processed.
Consider adding URL validation before processing:
+ import { isValidUrl } from "@galacean/engine-core";
import { AssetPromise, AssetType, Loader, LoadItem, resourceLoader, Script } from "@galacean/engine-core";
@resourceLoader(AssetType.Script, ["js", "mjs"], false)Committable suggestion was skipped due to low confidence.
packages/loader/src/ScriptLoader.ts
Outdated
| class ScriptLoader extends Loader<Script> { | ||
| load(item: LoadItem): AssetPromise<Script> { | ||
| return new AssetPromise((resolve, reject) => { | ||
| const { url } = item; | ||
| (import(url) as Promise<Script>) | ||
| .then((esModule) => { | ||
| resolve(esModule); | ||
| }) | ||
| .catch(reject); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance browser compatibility and error handling
The current implementation has several areas for improvement:
- No polyfill for dynamic import in older browsers
- Basic error handling without specific error types
- No cleanup mechanism for failed loads
Consider implementing these improvements:
class ScriptLoader extends Loader<Script> {
+ private loadingScripts: Map<string, Promise<Script>> = new Map();
+
load(item: LoadItem): AssetPromise<Script> {
return new AssetPromise((resolve, reject) => {
const { url } = item;
- (import(url) as Promise<Script>)
- .then((esModule) => {
- resolve(esModule);
- })
- .catch(reject);
+
+ // Reuse existing load promise if available
+ const existing = this.loadingScripts.get(url);
+ if (existing) {
+ return existing.then(resolve).catch(reject);
+ }
+
+ const loadPromise = (async () => {
+ try {
+ // Use polyfill for dynamic import
+ const moduleLoader = await import('dynamic-import-polyfill');
+ const esModule = await moduleLoader.load(url);
+ return esModule;
+ } catch (error) {
+ throw new Error(`Failed to load script ${url}: ${error.message}`);
+ } finally {
+ this.loadingScripts.delete(url);
+ }
+ })();
+
+ this.loadingScripts.set(url, loadPromise);
+ loadPromise.then(resolve).catch(reject);
});
}
+
+ // Cleanup method for resource management
+ destroy(): void {
+ this.loadingScripts.clear();
+ super.destroy?.();
+ }
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/ScriptLoader.ts (1)
8-9: Add class documentation and clarify decorator usageThe class lacks documentation explaining its purpose and usage. Additionally, the purpose of the
falseparameter in the decorator should be documented.+/** + * Loader implementation for JavaScript modules (ES modules). + * Supports both .js and .mjs file extensions. + */ @resourceLoader(AssetType.Script, ["js", "mjs"], false) export class ScriptLoader extends Loader<ESModuleStructure>tests/src/loader/ScriptLoader.test.ts (2)
7-10: Consider strengthening the interface type definition.The interface could be more specific about the allowed values for the index signature. Currently,
[key: string]: anyis too permissive and could mask type errors.Consider this improvement:
interface ESModuleStructure { default?: Script; - [key: string]: any; + [key: string]: Script | (() => string) | string | undefined; }
19-52: Add test coverage for error scenarios.The current tests only cover successful scenarios. Consider adding tests for:
- Invalid script content
- Network errors
- Script loading cancellation
- Invalid MIME types
- Circular dependencies
Would you like me to help generate these additional test cases?
🧰 Tools
🪛 eslint
[error] 52-52: Insert
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/loader/src/ScriptLoader.ts(1 hunks)packages/loader/src/index.ts(1 hunks)tests/src/loader/ScriptLoader.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loader/src/index.ts
[failure] 29-29:
Replace './ScriptLoader' with "./ScriptLoader";
🪛 eslint
packages/loader/src/index.ts
[error] 29-29: Replace './ScriptLoader' with "./ScriptLoader";
(prettier/prettier)
tests/src/loader/ScriptLoader.test.ts
[error] 52-52: Insert ⏎
(prettier/prettier)
🔇 Additional comments (2)
packages/loader/src/ScriptLoader.ts (1)
10-19: 🛠️ Refactor suggestion
Implement caching and improve error handling
The current implementation could benefit from:
- Caching mechanism to prevent redundant loads
- Detailed error messages for different failure scenarios
- Cleanup mechanism for failed loads
+private moduleCache = new Map<string, Promise<ESModuleStructure>>();
+
load(item: LoadItem): AssetPromise<ESModuleStructure> {
return new AssetPromise((resolve, reject) => {
const { url } = item;
+
+ // Return cached module if available
+ const cached = this.moduleCache.get(url);
+ if (cached) {
+ return cached.then(resolve).catch(reject);
+ }
+
+ const loadPromise = (async () => {
+ try {
(import(url) as Promise<ESModuleStructure>)
- .then((esModule) => {
- resolve(esModule);
- })
- .catch(reject);
+ .then((esModule) => {
+ if (!esModule || (typeof esModule !== 'object')) {
+ throw new Error(`Invalid module format from ${url}`);
+ }
+ return esModule;
+ });
+ } catch (error) {
+ this.moduleCache.delete(url);
+ throw new Error(`Failed to load script from ${url}: ${error.message}`);
+ }
+ })();
+
+ this.moduleCache.set(url, loadPromise);
+ loadPromise.then(resolve).catch(reject);
});
}
+
+/**
+ * Cleans up cached modules
+ */
+destroy(): void {
+ this.moduleCache.clear();
+ super.destroy?.();
+}Let's verify if there are any existing module caching implementations in the codebase:
packages/loader/src/index.ts (1)
22-22: LGTM!
The import statement follows the established pattern and is positioned appropriately with other loader imports.
packages/loader/src/ScriptLoader.ts
Outdated
| interface ESModuleStructure { | ||
| default?: Script; | ||
| [key: string]: any; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance type safety and documentation for ESModuleStructure interface
The interface could benefit from:
- Stricter typing instead of
any - JSDoc documentation explaining its purpose
- 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.
| 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; | |
| } |
| before(async () => { | ||
| const canvasDOM = document.createElement("canvas"); | ||
| canvasDOM.width = 1024; | ||
| canvasDOM.height = 1024; | ||
| engine = await WebGLEngine.create({ canvas: canvasDOM }); | ||
| }); |
There was a problem hiding this comment.
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.
| 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(); | |
| }); |
| 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.
Improve test reliability and security.
Several concerns with the current implementation:
- Using an external CDN in tests makes them dependent on network connectivity and third-party availability
- Missing error handling for failed requests
- 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) | ||
| }); | ||
| }); |
There was a problem hiding this comment.
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.
| 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); | |
| } | |
| }); |
| 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 }); | ||
| }); | ||
|
|
||
| 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; | ||
| }); | ||
| }); | ||
|
|
||
| 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.
💡 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:
- Scripts can only be loaded from valid URLs or Blob URLs
- Scripts are executed in strict mode by default
- Scripts inherit Content Security Policy (CSP) restrictions from the host page
However, the loader lacks:
- Script integrity validation (SRI)
- Origin/domain validation
- 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:
- Adding content security policies
- Implementing script integrity validation
- 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)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
tests/src/loader/ScriptLoader.test.ts (3)
8-11: Consider making the interface more strict.The
[key: string]: anyindex signature makes the interface too permissive. Consider explicitly listing all expected properties for better type safety.interface ESModuleStructure { default?: Script; - [key: string]: any; + colord: unknown; + getFormat: () => string; + random?: unknown; }
13-18: Improve test setup maintainability.Consider these improvements:
- Extract canvas dimensions as named constants
- Clean up the canvas DOM element in afterAll
+const CANVAS_WIDTH = 1024; +const CANVAS_HEIGHT = 1024; +let canvasDOM: HTMLCanvasElement; beforeAll(async () => { - const canvasDOM = document.createElement("canvas"); - canvasDOM.width = 1024; - canvasDOM.height = 1024; + canvasDOM = document.createElement("canvas"); + canvasDOM.width = CANVAS_WIDTH; + canvasDOM.height = CANVAS_HEIGHT; engine = await WebGLEngine.create({ canvas: canvasDOM }); });
55-59: Fix code style.Add missing semicolon after the last statement to maintain consistency.
afterAll(function () { engine.resourceManager.gc(); - engine.destroy(); + engine.destroy(); });🧰 Tools
🪛 eslint
[error] 59-59: Insert
⏎(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
tests/src/loader/ScriptLoader.test.ts(1 hunks)
🧰 Additional context used
🪛 eslint
tests/src/loader/ScriptLoader.test.ts
[error] 59-59: Insert ⏎
(prettier/prettier)
| 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.
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
‼️ 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.
| 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; | |
| }); | |
| }); | |
| it("loader from string url", async () => { | |
| const script = await engine.resourceManager.load<ESModuleStructure>({ | |
| url: "https://cdn.jsdelivr.net/npm/[email protected]/+esm", | |
| type: AssetType.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; | |
| }); |
| 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.
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
‼️ 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.
| 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 { | |
| const script = await engine.resourceManager.load<ESModuleStructure>({ | |
| url: blobUrl, | |
| type: AssetType.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); | |
| } finally { | |
| URL.revokeObjectURL(blobUrl); | |
| } | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/loader/src/ScriptLoader.ts (1)
13-13: Document the purpose of @vite-ignore.The
/* @vite-ignore */comment should be documented to explain why it's necessary. This helps maintainers understand the implications of removing it.- (import(/* @vite-ignore */url) as Promise<ESModuleStructure>) + // @vite-ignore is required to prevent Vite from trying to statically analyze dynamic imports + (import(/* @vite-ignore */ url) as Promise<ESModuleStructure>)🧰 Tools
🪛 GitHub Check: lint
[failure] 13-13:
Insert·
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loader/src/ScriptLoader.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
packages/loader/src/ScriptLoader.ts
[failure] 13-13:
Insert ·
🔇 Additional comments (3)
packages/loader/src/ScriptLoader.ts (3)
1-1: LGTM! Imports are well-organized and complete.
3-6: Reuse existing type safety comment.
10-19: Consider implementing the suggested dynamic-import-polyfill.
As mentioned in the PR comments, implementing the dynamic-import-polyfill would improve browser compatibility. This is especially important for environments that don't support dynamic imports natively.
Let's verify the browser compatibility requirements:
Additionally, the existing review comment about enhancing browser compatibility and error handling is still valid and should be addressed.
🧰 Tools
🪛 GitHub Check: lint
[failure] 13-13:
Insert ·
packages/loader/src/ScriptLoader.ts
Outdated
| @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.
🛠️ 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
‼️ 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.
| @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); | |
| }); | |
| } | |
| } | |
| @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((error) => { | |
| this.loadingScripts.delete(url); | |
| reject(error); | |
| }); | |
| }); | |
| } | |
| destroy(): void { | |
| this.loadingScripts.clear(); | |
| super.destroy?.(); | |
| } | |
| } |
🧰 Tools
🪛 GitHub Check: lint
[failure] 13-13:
Insert ·
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/loader/src/ScriptLoader.ts (2)
13-13: Document the purpose of the vite-ignore comment.The
/* @vite-ignore */comment should be documented to explain why it's necessary and what implications it has for production builds.+ // Ignore Vite's static analysis for dynamic imports since URLs are determined at runtime (import(/* @vite-ignore */ url) as Promise<ESModule>)
10-19: Enhance error handling and add retry mechanism.The current implementation could benefit from:
- More specific error handling with custom error types
- A retry mechanism for transient network failures
load(item: LoadItem): AssetPromise<ESModule> { + const maxRetries = 3; + const retryDelay = 1000; + + const loadWithRetry = async (retriesLeft: number): Promise<ESModule> => { + try { + return await import(/* @vite-ignore */ url) as ESModule; + } catch (error) { + if (retriesLeft > 0 && error instanceof TypeError) { // Network errors are TypeError + await new Promise(resolve => setTimeout(resolve, retryDelay)); + return loadWithRetry(retriesLeft - 1); + } + throw new Error(`Failed to load script ${url}: ${error.message}`); + } + }; + return new AssetPromise((resolve, reject) => { const { url } = item; - (import(/* @vite-ignore */ url) as Promise<ESModule>) - .then((esModule) => { - resolve(esModule); - }) - .catch(reject); + loadWithRetry(maxRetries).then(resolve).catch(reject); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/loader/src/ScriptLoader.ts(1 hunks)packages/loader/src/index.ts(1 hunks)rollup.config.js(1 hunks)tests/src/loader/ScriptLoader.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rollup.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/loader/src/index.ts
- tests/src/loader/ScriptLoader.test.ts
🔇 Additional comments (2)
packages/loader/src/ScriptLoader.ts (2)
1-2: LGTM! Imports are well-structured.
The necessary dependencies are properly imported from @galacean/engine-core.
1-20: Verify browser compatibility requirements.
The PR mentions considering dynamic-import-polyfill, but it's not implemented. Let's verify browser support requirements.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
tests/src/loader/ScriptLoader.test.ts (2)
9-14: Add error handling to engine setup.The engine setup could fail if WebGL is not supported. Consider adding try-catch and proper error handling:
beforeAll(async () => { + try { const canvasDOM = document.createElement("canvas"); canvasDOM.width = 1024; canvasDOM.height = 1024; engine = await WebGLEngine.create({ canvas: canvasDOM }); + } catch (error) { + console.error('Failed to initialize WebGL engine:', error); + throw error; + } });
1-58: Run code formatter to fix style issues.Multiple formatting issues were detected by the linter, including:
- Missing semicolons
- Inconsistent indentation
- Missing spaces
Consider running the project's code formatter or prettier to fix these issues.
🧰 Tools
🪛 eslint
[error] 18-19: Replace
⏎····engine.resourceManagerwithengine.resourceManager⏎······(prettier/prettier)
[error] 20-20: Insert
··(prettier/prettier)
[error] 21-21: Insert
··(prettier/prettier)
[error] 22-22: Insert
··(prettier/prettier)
[error] 23-23: Insert
··(prettier/prettier)
[error] 24-24: Insert
··(prettier/prettier)
[error] 25-25: Insert
··(prettier/prettier)
[error] 26-26: Insert
··(prettier/prettier)
[error] 27-27: Insert
··(prettier/prettier)
[error] 28-28: Insert
··(prettier/prettier)
[error] 29-29: Insert
··(prettier/prettier)
[error] 39-39: Insert
;(prettier/prettier)
[error] 40-40: Insert
⏎······(prettier/prettier)
[error] 41-41: Insert
··(prettier/prettier)
[error] 42-42: Insert
··(prettier/prettier)
[error] 43-43: Insert
··(prettier/prettier)
[error] 44-44: Insert
··(prettier/prettier)
[error] 45-45: Insert
··(prettier/prettier)
[error] 46-46: Insert
··(prettier/prettier)
[error] 47-47: Insert
··(prettier/prettier)
[error] 48-48: Insert
··(prettier/prettier)
[error] 49-49: Insert
··(prettier/prettier)
[error] 50-50: Replace
······expect(script.default).equal(script.colord)with········expect(script.default).equal(script.colord);(prettier/prettier)
[error] 51-51: Replace
····})with······});(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loader/src/ScriptLoader.ts(1 hunks)tests/src/loader/ScriptLoader.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/ScriptLoader.ts
🧰 Additional context used
🪛 eslint
tests/src/loader/ScriptLoader.test.ts
[error] 18-19: Replace ⏎····engine.resourceManager with engine.resourceManager⏎······
(prettier/prettier)
[error] 20-20: Insert ··
(prettier/prettier)
[error] 21-21: Insert ··
(prettier/prettier)
[error] 22-22: Insert ··
(prettier/prettier)
[error] 23-23: Insert ··
(prettier/prettier)
[error] 24-24: Insert ··
(prettier/prettier)
[error] 25-25: Insert ··
(prettier/prettier)
[error] 26-26: Insert ··
(prettier/prettier)
[error] 27-27: Insert ··
(prettier/prettier)
[error] 28-28: Insert ··
(prettier/prettier)
[error] 29-29: Insert ··
(prettier/prettier)
[error] 39-39: Insert ;
(prettier/prettier)
[error] 40-40: Insert ⏎······
(prettier/prettier)
[error] 41-41: Insert ··
(prettier/prettier)
[error] 42-42: Insert ··
(prettier/prettier)
[error] 43-43: Insert ··
(prettier/prettier)
[error] 44-44: Insert ··
(prettier/prettier)
[error] 45-45: Insert ··
(prettier/prettier)
[error] 46-46: Insert ··
(prettier/prettier)
[error] 47-47: Insert ··
(prettier/prettier)
[error] 48-48: Insert ··
(prettier/prettier)
[error] 49-49: Insert ··
(prettier/prettier)
[error] 50-50: Replace ······expect(script.default).equal(script.colord) with ········expect(script.default).equal(script.colord);
(prettier/prettier)
[error] 51-51: Replace ····}) with ······});
(prettier/prettier)
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature:
ScriptLoaderAssetType.ScriptWhat is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit
New Features
Scriptasset type for better asset management.ScriptLoaderclass for loading script assets efficiently, supporting both "js" and "mjs" file types.ScriptLoaderandESModulefor broader use.Bug Fixes
Tests
ScriptLoaderfunctionality, verifying script loading from both URL and Blob sources.