Refactor/audit reduce global state#5530
Open
vanshika2720 wants to merge 5 commits intosugarlabs:masterfrom
Open
Refactor/audit reduce global state#5530vanshika2720 wants to merge 5 commits intosugarlabs:masterfrom
vanshika2720 wants to merge 5 commits intosugarlabs:masterfrom
Conversation
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
1 similar comment
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
|
Please address the Lint Errors for |
vanshika2720
force-pushed
the
refactor/audit-reduce-global-state
branch
from
e51b96c to
1f1ccfe
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
vanshika2720
force-pushed
the
refactor/audit-reduce-global-state
branch
from
1f1ccfe to
fc76345
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
vanshika2720
force-pushed
the
refactor/audit-reduce-global-state
branch
from
fc76345 to
515ee2e
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
Author
|
@walterbender please have a review on this |
Member
|
I think this is a nice approach. As per the question I raised in the PhraseMaker widget refactoring, do we want to go with this._deps.... everywhere (Phase 4 as described in your md file) or just define the relevant consts so as to not require major changes. |
walterbender
added
the
WF3-Needs design review
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
vanshika2720
force-pushed
the
refactor/audit-reduce-global-state
branch
from
933e4fc to
ad66917
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
3 similar comments
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
This commit introduces explicit dependency injection for the Logo subsystem to reduce reliance on the global Activity facade. Included is an audit of current dependencies and unit tests demonstrating the new pattern.
This refactor follows the dependency injection pattern established for the Logo subsystem, making PhraseMaker's external dependencies explicit and testable. ## Changes Made ✅ Introduced explicit dependency injection via constructor ✅ Updated instantiation site in WidgetBlocks.js to pass dependencies ✅ Replaced all global function calls with this._deps.* pattern (289 refs) ✅ Verified runtime behavior with Jest (all tests passing) ✅ Created a clean, auditable isolation boundary ## Design Decision: this._deps.* vs Local Constants This implementation uses `this._deps.*` throughout rather than extracting local constants (e.g., `const _ = this._deps._`) for the following reasons: 1. **Explicit Dependency Tracking**: Every `this._deps.*` call makes it immediately clear that we're using an injected dependency, not a global. This aids in auditing and understanding data flow. 2. **Grep-ability**: Searching for `this._deps.` instantly reveals all external dependencies used in any method, making refactoring safer. 3. **Consistency with Logo Pattern**: Matches the approach documented in GLOBAL_STATE_AUDIT.md Phase 4, establishing a uniform pattern across the codebase. 4. **Future-Proof**: If we later need to swap implementations or add instrumentation, having explicit `this._deps.*` calls makes it easier to intercept or modify behavior. 5. **No Hidden Globals**: Local constants like `const _ = this._deps._` could be mistaken for module-level imports, reducing clarity. The trade-off is more verbose code, but the architectural benefits of explicit, traceable dependencies outweigh the verbosity cost. ## Testing All 2430 Jest tests pass. No behavioral changes. Addresses sugarlabs#5529 (Audit and Reduce Implicit Global State Usage)
Automated formatting fixes to comply with Prettier code style. No functional changes.
Apply local binding pattern to Logo and PhraseMaker to improve code readability while maintaining explicit dependency injection. ## Changes Made ✅ Logo: Bind blocks, turtles, stage as local properties ✅ PhraseMaker: Bind platformColor, docById, _, wheelnav, slicePath locally ✅ Replace this._deps.* with this.* for bound dependencies (161 refs in PhraseMaker) ✅ Update GLOBAL_STATE_AUDIT.md Phase 4 to document this pattern ✅ All tests passing (2430 tests) ## Pattern Benefits 1. **Reduced Verbosity**: `this.blocks` instead of `this.deps.blocks` 2. **Maintained Auditability**: All dependencies declared in constructor 3. **Grep-able**: Easy to search for `this.blocks` to find usages 4. **No Behavior Change**: Functionally equivalent to `this.deps.*` 5. **Explicit Contracts**: Dependencies still injected, not global This addresses reviewer feedback about excessive `this._deps.*` usage while preserving the architectural benefits of dependency injection. Addresses sugarlabs#5529 (Audit and Reduce Implicit Global State Usage)
vanshika2720
force-pushed
the
refactor/audit-reduce-global-state
branch
from
7b3607b to
14fe9ae
Compare
Contributor
|
✅ All Jest tests passed! This PR is ready to merge. |
Contributor
Author
|
@walterbender I’ve simplified the usage without changing the architecture:
|
Member
|
This looks good to me. Should we close the PhraseMaker PR? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architecture Pilot: Audit & Reduce Implicit Global State (Logo Subsystem)
Summary
This PR introduces a pilot architectural improvement to audit and reduce implicit global state usage in Music Blocks by focusing on the Logo subsystem. It establishes a repeatable, backward-compatible pattern for making dependencies explicit without changing runtime behavior.
This work builds directly on the goals of #2767 and is intentionally scoped as a non-breaking, reference implementation rather than a full refactor.
What’s Included
1. Global State Audit
this.activity.*2. Explicit Dependency Interface
LogoDependenciesclass defining clear dependency contractsfromActivity()factory for backward compatibility with existing architecture3. Backward-Compatible Constructor Refactor
Logoto accept either:Activityobject (unchanged behavior), orLogoDependenciesinstance (new explicit pattern)4. Testability Demonstration
5. Implementation Documentation
Why This Matters
Verification
logo.test.js)Scope (Intentional)
Included:
Not included (future work):
this.activity.*usagesRelated Issues
[Chore] Audit and Reduce Implicit Global State Usage (Architecture-Focused) #5529