refactor(utils): replace eval-based dynamic resolution with resolveObject helper#4995
Conversation
|
✅ All Jest tests passed! This PR is ready to merge. |
There was a problem hiding this comment.
Pull request overview
This PR refactors dynamic object resolution in js/utils/utils.js by introducing a resolveObject helper function to replace eval() calls used for dot-notation property access. The change aims to improve code safety, readability, and maintainability without altering runtime behavior.
Key changes:
- Added
resolveObjecthelper to safely resolve dot-notation paths (e.g., "MyClass.Model") to global objects - Replaced 3
eval()calls inimportMemberswithresolveObjectfor class/property lookups - Changed one
eval()call tonew Function()in plugin execution code
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WillyEverGreen
force-pushed
the
refactor-eval-fix
branch
from
2808927 to
9a962fe
Compare
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
All feedback has been addressed: PR scope narrowed to resolveObject refactor only |
|
It doesn't load for me... Firefox/Fedora 42. Also, please test i18n as this change impacts Japanese. |
|
✅ All Jest tests passed! This PR is ready to merge. |
Fixed! Added better error handling to resolveObject (type checking, null handling, try/catch). Tested Japanese i18n, works correctly. All tests pass. Ready for review. |
|
It doesn't open for me: |
Thanks for the report. I'm investigating this now. The error occurs when importMembers tries to resolve Turtles.TurtlesView during initialization. Debugging locally to identify why the nested property resolution is failing, and I’ll follow up once I’ve confirmed the cause and next steps. |
The resolveObject helper failed to resolve certain nested static class properties (e.g. Turtles.TurtlesView), resulting in missing methods such as doScale during initialization. This change adds a narrow fallback for cases where safe property resolution returns undefined, restoring previous behavior while keeping the primary resolution path intact. Addresses initialization failure reported during review.
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
The issue was that Tested locally:
|
…ject helper (sugarlabs#4995) * refactor: replace unsafe eval with resolveObject helper in utils.js * refactor: replace safeEval's eval() with new Function() * revert: keep original eval in safeEval, focus PR on resolveObject only * refactor: move resolveObject to module scope * fix(utils): improve resolveObject error handling and environment support * fix(utils): add fallback resolution for nested class properties The resolveObject helper failed to resolve certain nested static class properties (e.g. Turtles.TurtlesView), resulting in missing methods such as doScale during initialization. This change adds a narrow fallback for cases where safe property resolution returns undefined, restoring previous behavior while keeping the primary resolution path intact. Addresses initialization failure reported during review. * style: apply prettier formatting to utils.js
Summary
This PR replaces a small number of
eval()calls injs/utils/utils.jswith a safer helper function (resolveObject) that resolves dot-notation strings (e.g.,"MyClass.Model") to global objects without executing arbitrary code.The change preserves existing behavior while improving readability, safety, and maintainability.
Background
eval()is a JavaScript function that executes a string as code at runtime. While it can be useful in certain situations, it also:In this file,
eval()was not being used to run dynamic logic, but only to resolve class or property names dynamically (for example, converting a string like"Foo.Bar"intowindow.Foo.Bar).What Changed
resolveObject(path)helper injs/utils/utils.jseval()used for dynamic class/property access insideinstantiateComponentjs/utils/utils.jsThis change is limited in scope and does not alter runtime behavior.
Why This Approach
Using a dedicated resolver function instead of
eval():Testing
npm run lint— passed