Polyfill String.matchAll for compatibility
#2443
Conversation
Sway007
added
high priority
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (3)
WalkthroughThe changes involve updates to the Changes
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2443 +/- ##
==========================================
- Coverage 70.78% 70.73% -0.06%
==========================================
Files 524 526 +2
Lines 27487 27504 +17
Branches 4111 4113 +2
==========================================
- Hits 19457 19454 -3
- Misses 6477 6495 +18
- Partials 1553 1555 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![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 (1)
packages/loader/src/ShaderChunkLoader.ts (1)
15-15: LGTM! Consider adding documentation.The regex pattern correctly identifies shader includes. Consider adding a comment explaining the pattern components for better maintainability.
- private static _shaderIncludeRegex = /\s#include\s+"([./][^\\"]+)"/; + /** Matches shader include statements in the format: #include "./path/to/shader" */ + private static _shaderIncludeRegex = /\s#include\s+"([./][^\\"]+)"/;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loader/src/ShaderChunkLoader.ts(2 hunks)
🧰 Additional context used
🪛 Biome
packages/loader/src/ShaderChunkLoader.ts
[error] 20-20: void is confusing outside a return type or a type parameter.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 GitHub Check: codecov/patch
packages/loader/src/ShaderChunkLoader.ts
[warning] 22-22: packages/loader/src/ShaderChunkLoader.ts#L22
Added line #L22 was not covered by tests
[warning] 44-45: packages/loader/src/ShaderChunkLoader.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 47-47: packages/loader/src/ShaderChunkLoader.ts#L47
Added line #L47 was not covered by tests
[warning] 49-51: packages/loader/src/ShaderChunkLoader.ts#L49-L51
Added lines #L49 - L51 were not covered by tests
[warning] 53-53: packages/loader/src/ShaderChunkLoader.ts#L53
Added line #L53 was not covered by tests
🔇 Additional comments (1)
packages/loader/src/ShaderChunkLoader.ts (1)
43-54: Verify browser compatibility requirements.
Before implementing a polyfill, let's verify if it's necessary based on the engine's browser support requirements.
Consider using existing polyfill libraries like core-js if broader compatibility is needed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 44-45: packages/loader/src/ShaderChunkLoader.ts#L44-L45
Added lines #L44 - L45 were not covered by tests
[warning] 47-47: packages/loader/src/ShaderChunkLoader.ts#L47
Added line #L47 was not covered by tests
[warning] 49-51: packages/loader/src/ShaderChunkLoader.ts#L49-L51
Added lines #L49 - L51 were not covered by tests
[warning] 53-53: packages/loader/src/ShaderChunkLoader.ts#L53
Added line #L53 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (8)
packages/loader/src/index.ts (1)
24-24: Consider grouping utility imports together.The import looks good, but consider moving it up with other utility imports (if any) for better organization.
packages/loader/src/Polyfill.ts (3)
3-7: Consider using standalone functions instead of a static classWhile the current implementation using a static class works, consider refactoring to use standalone functions for a more functional approach. However, if you plan to add more polyfills in the future, the class can serve as a good namespace.
-export class Polyfill { - static registerPolyFill() { - Polyfill._registerMatchAll(); - } -} +export function registerPolyfills() { + registerMatchAllPolyfill(); +}🧰 Tools
🪛 Biome
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
8-32: Add unit tests for the polyfill implementationThe PR objectives mention that tests haven't been added yet. This polyfill needs comprehensive testing to ensure compatibility.
Would you like me to help create a test suite that covers:
- Basic matching functionality
- Capture groups handling
- Error cases (non-global regex)
- Edge cases (empty strings, no matches)
9-10: Consider adding browser compatibility commentAdd a comment indicating which browsers/versions need this polyfill for better maintainability.
if (!String.prototype.matchAll) { + // Polyfill needed for: IE11, older versions of Safari (<13), etc. Logger.info("polyfill String.prototype.matchAll");tests/src/loader/Polyfill.test.ts (4)
5-12: Consider adding JSDoc comments for the test patternsAdding documentation for the regex pattern and test content would improve maintainability and help other developers understand the test cases better.
+ /** Regex pattern to match GLSL include statements with relative or absolute paths */ const regex = /#include\s+"([./][^\\"]+)"/gm; + /** Sample GLSL content with various include patterns to test */ const content = `#include "./f1.glsl"
14-15: Add error handling for module importThe import could fail if the module is not available. Consider wrapping it in a try-catch block.
String.prototype.matchAll = null; - await import("@galacean/engine-loader"); + try { + await import("@galacean/engine-loader"); + } catch (error) { + throw new Error("Failed to load polyfill module: " + error.message); + }
17-22: Consider adding more error casesThe test covers the missing global flag case, but consider adding tests for other edge cases:
- null/undefined regex
- invalid regex patterns
- empty string content
Would you like me to provide example test cases for these scenarios?
3-39: Consider splitting the test into smaller, focused test casesThe current test contains multiple assertions testing different aspects of the polyfill. Consider splitting it into separate test cases:
- Basic functionality test
- Error handling test
- Edge cases test
This would improve test maintainability and make failures easier to diagnose.
🧰 Tools
🪛 Biome
[error] 25-25: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/loader/src/Polyfill.ts(1 hunks)packages/loader/src/ShaderChunkLoader.ts(1 hunks)packages/loader/src/index.ts(2 hunks)tests/src/loader/Polyfill.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/ShaderChunkLoader.ts
🧰 Additional context used
🪛 Biome
packages/loader/src/Polyfill.ts
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
tests/src/loader/Polyfill.test.ts
[error] 25-25: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (2)
packages/loader/src/index.ts (1)
37-37: Reconsider the placement and implementation of polyfill registration.
The unconditional polyfill registration in a barrel file raises several concerns:
- Multiple registrations could occur if this file is imported multiple times
- Side effects in barrel files can lead to unexpected behavior
- No clear indication why this specific polyfill is needed at this level
Consider these alternatives:
- Move the registration to the application entry point
- Add a guard to prevent multiple registrations:
-Polyfill.registerPolyFill();
+if (!String.prototype.matchAll) {
+ Polyfill.registerPolyFill();
+}- Document why this polyfill is necessary at this level
Let's verify the potential impact:
packages/loader/src/Polyfill.ts (1)
1-33: Verify all String.matchAll usage patterns
Let's verify all usage patterns of String.matchAll in the codebase to ensure this polyfill meets all requirements.
✅ Verification successful
Let me gather more context about the test cases to ensure the polyfill implementation matches the expected behavior.
Polyfill implementation correctly handles all usage patterns
The polyfill implementation has been verified against all its usages in the codebase:
- Test cases explicitly verify the polyfill's behavior with global regex flag requirement
- Test cases validate correct capture group handling and iteration
- The only production usage in ShaderChunkLoader matches the tested pattern (#include regex with capture groups)
- The implementation correctly throws TypeError for non-global regex as per spec
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all String.matchAll usage patterns
rg -A 2 "\.matchAll\("
# Search for potential regex patterns that might be used with matchAll
ast-grep --pattern 'new RegExp($$$, $$$)'
Length of output: 1140
Script:
#!/bin/bash
# Get the test file content to understand the test cases
cat tests/src/loader/Polyfill.test.ts
# Get the shader chunk loader implementation to understand its usage
cat packages/loader/src/ShaderChunkLoader.ts
Length of output: 2982
🧰 Tools
🪛 Biome
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/Polyfill.ts (3)
1-3: Consider refactoring static class to moduleThe class contains only static members which is an anti-pattern in TypeScript. Consider refactoring to a module with named exports for better maintainability.
import { Logger } from "@galacean/engine-core"; -export class Polyfill { +export const registerPolyfill = () => { + registerMatchAll(); +}; + +const registerMatchAll = () => {
4-6: Fix method name casingThe method name
registerPolyFilluses inconsistent casing. In JavaScript/TypeScript, camelCase is the convention for method names.- static registerPolyFill() { + static registerPolyfill() {
11-11: Fix return type annotationThe return type annotation uses
Stringinstead of the primitivestringtype. TypeScript best practices recommend using lowercase primitive types.- String.prototype.matchAll = function (pattern: RegExp): ReturnType<String["matchAll"]> { + String.prototype.matchAll = function (pattern: RegExp): ReturnType<string["matchAll"]> {🧰 Tools
🪛 Biome
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loader/src/Polyfill.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/loader/src/Polyfill.ts
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/Polyfill.ts (3)
3-6: Consider refactoring to module-level functionsThe class currently only contains static methods. Consider refactoring to module-level functions for better maintainability and simpler structure.
-export class Polyfill { - static registerPolyfill() { - Polyfill._registerMatchAll(); - } +export function registerPolyfill() { + _registerMatchAll(); +}🧰 Tools
🪛 Biome
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
8-32: Add JSDoc documentation for the polyfill implementationAdd comprehensive JSDoc documentation to explain:
- Purpose of the polyfill
- Browser compatibility requirements
- Parameters and return type
- Usage examples
+/** + * Polyfills the String.prototype.matchAll method for browsers that don't support it. + * @see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll + * + * @example + * const str = 'test1test2'; + * const regexp = /t(e)(st(\d?))/g; + * const matches = str.matchAll(regexp); + * for (const match of matches) { + * console.log(match); + * } + */ private static _registerMatchAll() {🧰 Tools
🪛 Biome
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
🪛 GitHub Check: codecov/patch
[warning] 10-13: packages/loader/src/Polyfill.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 15-15: packages/loader/src/Polyfill.ts#L15
Added line #L15 was not covered by tests
[warning] 17-17: packages/loader/src/Polyfill.ts#L17
Added line #L17 was not covered by tests
[warning] 19-20: packages/loader/src/Polyfill.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-23: packages/loader/src/Polyfill.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-27: packages/loader/src/Polyfill.ts#L25-L27
Added lines #L25 - L27 were not covered by tests
11-11: Use lowercase primitive typeReplace
Stringtype with lowercasestringfor consistency with TypeScript best practices.- String.prototype.matchAll = function (pattern: RegExp): ReturnType<String["matchAll"]> { + String.prototype.matchAll = function (pattern: RegExp): ReturnType<string["matchAll"]> {🧰 Tools
🪛 Biome
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/loader/src/Polyfill.ts(1 hunks)packages/loader/src/index.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/loader/src/index.ts
🧰 Additional context used
🪛 Biome
packages/loader/src/Polyfill.ts
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 GitHub Check: codecov/patch
packages/loader/src/Polyfill.ts
[warning] 10-13: packages/loader/src/Polyfill.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 15-15: packages/loader/src/Polyfill.ts#L15
Added line #L15 was not covered by tests
[warning] 17-17: packages/loader/src/Polyfill.ts#L17
Added line #L17 was not covered by tests
[warning] 19-20: packages/loader/src/Polyfill.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-23: packages/loader/src/Polyfill.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-27: packages/loader/src/Polyfill.ts#L25-L27
Added lines #L25 - L27 were not covered by tests
🔇 Additional comments (1)
packages/loader/src/Polyfill.ts (1)
1-33: Add test coverage for the polyfill
The code coverage report indicates missing test coverage. Please add tests to verify:
- Basic functionality with global RegExp
- Error handling for non-global RegExp
- Edge cases (null matches, empty strings)
- Compatibility with the native implementation
🧰 Tools
🪛 Biome
[error] 3-33: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 11-11: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🪛 GitHub Check: codecov/patch
[warning] 10-13: packages/loader/src/Polyfill.ts#L10-L13
Added lines #L10 - L13 were not covered by tests
[warning] 15-15: packages/loader/src/Polyfill.ts#L15
Added line #L15 was not covered by tests
[warning] 17-17: packages/loader/src/Polyfill.ts#L17
Added line #L17 was not covered by tests
[warning] 19-20: packages/loader/src/Polyfill.ts#L19-L20
Added lines #L19 - L20 were not covered by tests
[warning] 22-23: packages/loader/src/Polyfill.ts#L22-L23
Added lines #L22 - L23 were not covered by tests
[warning] 25-27: packages/loader/src/Polyfill.ts#L25-L27
Added lines #L25 - L27 were not covered by tests
packages/loader/src/Polyfill.ts
Outdated
| @@ -0,0 +1,33 @@ | |||
| import { Logger } from "@galacean/engine-core"; | |||
|
|
|||
| export class Polyfill { | |||
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/loader/src/Polyfill.ts (3)
3-6: Enhance documentation with more details.The
@internalJSDoc comment should provide more context about the purpose and usage of this polyfill.Add more details to the documentation:
/** * @internal + * Provides polyfills for missing String prototype methods in older JavaScript environments. + * Currently implements: + * - String.prototype.matchAll: Adds support for iterating over regex matches with capture groups */
6-10: Consider converting static-only class to namespace functions.The
Polyfillclass only contains static members. Consider using a more functional approach with a namespace or individual functions.Example refactor:
-export class Polyfill { - static registerPolyfill(): void { - Polyfill._registerMatchAll(); - } +export const registerPolyfills = (): void => { + registerMatchAll(); +}; + +const registerMatchAll = (): void => {🧰 Tools
🪛 Biome
[error] 6-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
14-14: Use lowercase primitive type.Replace
Stringwith lowercasestringin the return type for consistency with TypeScript conventions.- String.prototype.matchAll = function (pattern: RegExp): ReturnType<String["matchAll"]> { + String.prototype.matchAll = function (pattern: RegExp): ReturnType<string["matchAll"]> {🧰 Tools
🪛 Biome
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead(lint/complexity/noBannedTypes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/loader/src/Polyfill.ts(1 hunks)
🧰 Additional context used
🪛 Biome
packages/loader/src/Polyfill.ts
[error] 6-36: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 14-14: Don't use 'String' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'string' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (1)
packages/loader/src/Polyfill.ts (1)
17-19: LGTM! Proper error handling for non-global RegExp.
The implementation correctly validates the global flag requirement and throws an appropriate TypeError, matching the standard behavior.
Please check if the PR fulfills these requirements
The builtin function
string.matchAllhas version compatibility issues.Summary by CodeRabbit
New Features
String.prototype.matchAll, enhancing string matching capabilities in unsupported environments.Bug Fixes
matchAllmethod throws aTypeErrorwhen the global flag is not used.Tests