feat: access providers in same scope #32
Conversation
WalkthroughAdds provider debugName support, refactors ProviderScope initialization to allow same-scope provider dependencies with declaration-order enforcement and forward-reference detection, renames/unifies scope errors, adds benchmarks and CI workflow, and updates docs, tests, and analysis options to match the new provider ordering semantics. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProviderScope
participant ProviderA as Provider A
participant ProviderB as Provider B
rect rgb(240,248,255)
Note over ProviderScope: Phase 1 — registration & indexing
Client->>ProviderScope: initState()
ProviderScope->>ProviderScope: register providers, assign indices
end
rect rgb(240,255,240)
Note over ProviderScope: Phase 2 — eager creation
ProviderScope->>ProviderA: create if non-lazy
ProviderA-->>ProviderScope: value cached
end
rect rgb(255,250,230)
Note over ProviderScope: Phase 3 — lazy/on-demand creation with same-scope checks
Client->>ProviderScope: request ProviderB
ProviderScope->>ProviderScope: set _currentlyCreatingProviderIndex(B)
alt A declared before B
ProviderScope->>ProviderA: get (cached or create)
ProviderA-->>ProviderScope: value
ProviderScope->>ProviderB: create using ProviderA
ProviderB-->>ProviderScope: success
ProviderScope-->>Client: return value
else B declared before A
ProviderScope-->>Client: throw ProviderForwardReferenceError
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #32 +/- ##
=======================================
Coverage 99.53% 99.53%
=======================================
Files 9 9
Lines 217 217
=======================================
Hits 216 216
Misses 1 1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/disco/test/disco_test.dart (1)
171-195: Stale test description.The test description at line 172 still references
ArgProviderWithoutScopeError, but the assertion correctly uses the renamedProviderWithoutScopeError. Update the description for consistency.testWidgets( - '''Test ProviderScope throws ArgProviderWithoutScopeError for a not found ArgProvider''', + '''Test ProviderScope throws ProviderWithoutScopeError for a not found ArgProvider''', (tester) async {
🧹 Nitpick comments (2)
packages/disco/lib/src/utils/extensions.dart (2)
3-10: Return type is unnecessarily nullable.The getter always returns a non-null
String(line 6 initializessunconditionally), but the return type is declared asString?. This can mislead callers into thinking the value might be null.Apply this diff to correct the return type:
extension _DebugNameProvider<T extends Object> on Provider<T> { // Returns a debug name for the provider. - String? get _debugName { + String get _debugName { var s = 'Provider<$_valueType>'; if (debugName != null) s += '(name: $debugName)'; return s; } }
12-19: Same nullable return type issue.Same as above - this getter always returns a non-null
String.extension _DebugNameArgProvider<T extends Object, A> on ArgProvider<T, A> { // Returns a debug name for the provider with arguments. - String? get _debugName { + String get _debugName { var s = 'ArgProvider<$_valueType, $_argumentType>'; if (debugName != null) s += '(name: $debugName)'; return s; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
examples/auto_route/analysis_options.yaml(1 hunks)examples/bloc/analysis_options.yaml(1 hunks)examples/solidart/analysis_options.yaml(1 hunks)packages/disco/CHANGELOG.md(1 hunks)packages/disco/lib/src/disco_internal.dart(1 hunks)packages/disco/lib/src/models/overrides/provider_argument_override.dart(2 hunks)packages/disco/lib/src/models/providers/provider.dart(3 hunks)packages/disco/lib/src/models/providers/provider_argument.dart(4 hunks)packages/disco/lib/src/utils/extensions.dart(1 hunks)packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)packages/disco/pubspec.yaml(1 hunks)packages/disco/test/disco_test.dart(4 hunks)
🔇 Additional comments (21)
examples/auto_route/analysis_options.yaml (1)
4-4: Configuration change to suppress analyzer warning across examples.Adding
specify_nonobvious_property_typessuppression is consistent with updates to other example analysis_options.yaml files in this PR, likely due to type inference complexity in the new provider API.examples/solidart/analysis_options.yaml (1)
6-6: Configuration change consistent with other examples.Adding the same analyzer suppression rule to solidart example aligns with updates across examples in this PR.
examples/bloc/analysis_options.yaml (1)
1-3: Verify structural reordering intention.Unlike the other examples, this file restructures the YAML by moving the
analyzersection before theincludestatement. While the addition ofspecify_nonobvious_property_types: ignoreis consistent with other examples, please verify that reordering the sections was intentional, as YAML configuration order can impact how rules are applied.packages/disco/pubspec.yaml (1)
3-3: LGTM!Major version bump to 2.0.0 is appropriate given the breaking API changes (error type renames) and significant new features (same-scope provider dependencies, debugName parameter).
packages/disco/lib/src/disco_internal.dart (1)
13-13: LGTM!The new
extensions.dartpart directive follows the existing pattern and is logically placed among the utility parts.packages/disco/CHANGELOG.md (1)
1-4: LGTM!Changelog entries accurately document the new features and follow the established format.
packages/disco/lib/src/models/providers/provider.dart (3)
39-39: LGTM!The
debugNameparameter is correctly added to theProviderconstructor as an optional field.
50-52: LGTM!The
debugNameparameter is properly propagated through thewithArgumentfactory toArgProvider._.
123-127: LGTM!The
debugNamefield declaration with documentation follows the class pattern and provides clear purpose documentation.packages/disco/test/disco_test.dart (3)
517-571: LGTM!Good test coverage for the
ProviderScopePortalpath with lazy providers accessing same-scope providers, testing the cached value return and portal context navigation.
858-912: LGTM!Tests correctly validate:
- Non-lazy provider can access an earlier provider in the same scope
ProviderForwardReferenceErroris thrown when a provider references a later provider (wrong order)
1024-1067: LGTM!Excellent test for ensuring lazy provider values are cached and reused. The
creationCountassertion verifies the provider factory is only called once even when accessed by multiple dependent providers.packages/disco/lib/src/models/providers/provider_argument.dart (4)
20-20: LGTM!The
debugNameparameter is correctly added to theArgProvider._constructor.
40-41: LGTM!Correctly propagating
debugNametoArgProviderOverride._ensures debug names are preserved through override chains.
50-52: LGTM!The error type change from
ArgProviderWithoutScopeErrortoProviderWithoutScopeErrorunifies error handling across bothProviderandArgProvider, which is a cleaner API design.
85-87: LGTM!The
debugNamefield declaration follows the same pattern as inProvider, maintaining consistency across the codebase.packages/disco/lib/src/models/overrides/provider_argument_override.dart (1)
7-9: LGTM!The
debugNameparameter is properly added with appropriate nullability and immutability. The implementation follows the existing patterns in the codebase.Also applies to: 25-27
packages/disco/lib/src/widgets/provider_scope.dart (4)
221-235: Static scope tracking looks correct for typical usage.The static
_currentlyInitializingScopewith try/finally cleanup is appropriate for Flutter's synchronous widget initialization. The save/restore pattern increateProviderValue(lines 440-454) correctly handles lazy provider creation.
281-338: Two-phase initialization correctly separates registration from creation.Phase 1 registers all providers and tracks indices before Phase 2 creates non-lazy values. This ensures
isProviderInScope()works correctly during creation. The try/finally block properly cleans up static state.
646-658: Unified error handling for both provider types looks good.The
Objecttype with runtime switch pattern enables consistent error messaging for bothProviderandArgProvider. The fallback exception (line 653) provides a clear failure path if an unexpected type is encountered.
435-455: Save/restore pattern for lazy provider creation is well implemented.The try/finally correctly saves and restores scope tracking state, enabling lazy providers to access earlier same-scope providers while maintaining proper cleanup on exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/disco/lib/src/widgets/provider_scope.dart (1)
675-741: LGTM! Well-designed error types with clear messaging.The renamed
MultipleProviderOverrideOfSameInstanceimproves clarity. The newProviderForwardReferenceErroris particularly well-designed, providing comprehensive context (indices, provider references) and an actionable error message that explains both the problem and the solution.Note: There's a typo in the error message at line 734 (missing closing backtick) that was already flagged in a previous review.
packages/disco/test/disco_test.dart (1)
859-1221: Excellent comprehensive test coverage for same-scope provider access.The test suite thoroughly exercises the new same-scope provider access feature, covering:
- Both lazy and non-lazy providers
- Provider and ArgProvider interactions
- Nested dependencies (A→B→C chains)
- Value reuse/caching when multiple providers access the same earlier provider
- Forward reference error scenarios
- Mixed Provider and ArgProvider ordering
This coverage ensures the feature works correctly across various scenarios and properly detects ordering violations.
Note: The test at lines 1189-1221 has a description mismatch already flagged in a previous review.
🧹 Nitpick comments (1)
packages/disco/lib/src/widgets/provider_scope.dart (1)
241-411: Consider extracting helper methods to improve readability.The
initStatemethod is quite long (~170 lines) and handles multiple responsibilities: validation, two-phase provider registration, and override processing. While functionally correct, extracting helper methods would improve maintainability and testability.Consider refactoring into smaller methods:
void initState() { super.initState(); _currentlyInitializingScope = this; try { if (widget.providers != null) { _initializeProviders(widget.providers!); } else if (widget.overrides != null) { _initializeOverrides(widget.overrides!); } } finally { _currentlyInitializingScope = null; _currentlyCreatingProviderIndex = null; } } void _initializeProviders(List<InstantiableProvider> allProviders) { _validateProvidersUniqueness(allProviders); _registerAllProviders(allProviders); _createNonLazyProviders(allProviders); } void _initializeOverrides(List<Override> overrides) { _processProviderOverrides(overrides); _processArgProviderOverrides(overrides); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)packages/disco/test/disco_test.dart(4 hunks)
🔇 Additional comments (9)
packages/disco/lib/src/widgets/provider_scope.dart (6)
31-36: LGTM! Clear documentation of ordering requirements.The documentation clearly explains the ordering constraint for same-scope provider dependencies and references the error type that will be thrown. This will help developers understand the design decision upfront.
63-141: Good refactoring to centralize provider value creation logic.The centralized
_getOrCreateValuehelper effectively reduces code duplication betweenProviderandArgProvideraccess paths while enabling consistent same-scope dependency checking. The two-phase approach (check initializing scope, then search ancestors) is logical and well-structured.
435-455: LGTM! Proper save/restore pattern for lazy provider creation.The save/restore pattern for initialization state correctly enables same-scope dependency checking for lazy providers. The try-finally ensures proper cleanup even if value creation throws.
474-497: LGTM! Consistent save/restore pattern for ArgProvider.The same save/restore pattern is correctly applied to
ArgProviderlazy creation, maintaining consistency with theProviderimplementation.
646-658: LGTM! Improved error type with dynamic naming.The updated error type now accepts any provider type and uses dynamic naming via the new
debugNamesupport. The switch expression cleanly handles bothProviderandArgProvidercases with appropriate fallback for unexpected types.
221-235: The static_currentlyInitializingScopeis correctly handled for nested initialization. Lines 440-453 implement a proper save/restore pattern that preserves and restores the previous scope state, preventing any premature overwriting during nested initialization. Existing tests at lines 445-500 ("Test ProviderScope inside ProviderScopePortal works") and line 998 ("Nested provider dependencies work") already verify that nested scope initialization with cross-scope provider access works correctly. No changes are needed.packages/disco/test/disco_test.dart (3)
190-191: LGTM! Correctly updated to use unified error type.The test correctly reflects the API change from
ArgProviderWithoutScopeErrorto the unifiedProviderWithoutScopeErrortype with theproviderfield.
517-571: LGTM! Good test coverage for ProviderScopePortal with same-scope dependencies.This test effectively covers the interaction between
ProviderScopePortal, lazy providers, and same-scope dependencies. It verifies that lazy providers can access earlier providers in the same scope even when accessed through a portal context.
826-826: LGTM! Updated to match renamed error type.Correctly updated to expect
MultipleProviderOverrideOfSameInstancematching the renamed error type.
|
@manuel-plavsic let me know if you can review the PR given that I already tried to merge something similar in the past but failed. PS: I bumped a major, reaching v2 because I want to create another PR to create a new package |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/disco/lib/src/widgets/provider_scope.dart (2)
462-482: Correct same-scope support for lazy providers.The save/restore pattern for
_currentlyInitializingScopeand_currentlyCreatingProviderIndexis essential for nested lazy provider creation. The try-finally ensures proper restoration even if creation throws.The save/restore pattern is duplicated in both
createProviderValue(lines 467-481) andcreateProviderValueForArgProvider(lines 509-523). Consider extracting this pattern into a helper method to reduce duplication:T _withProviderIndex<T>(int? index, T Function() create) { final savedScope = _currentlyInitializingScope; final savedIndex = _currentlyCreatingProviderIndex; try { _currentlyInitializingScope = this; _currentlyCreatingProviderIndex = index; return create(); } finally { _currentlyInitializingScope = savedScope; _currentlyCreatingProviderIndex = savedIndex; } }However, since it's only used in two places, the current approach is acceptable.
442-452: Consider clearing index maps in dispose for consistency.The
disposemethod clears the main hash maps but not the new index maps (_providerIndicesand_argProviderIndices). While these will be garbage collected when the State object is disposed, explicitly clearing them would be more consistent with the existing cleanup pattern.Add these lines in dispose for consistency:
@override void dispose() { // dispose all the created providers createdProviderValues.forEach((key, value) { key._safeDisposeValue(value); }); allArgProvidersInScope.clear(); allProvidersInScope.clear(); createdProviderValues.clear(); _providerIndices.clear(); _argProviderIndices.clear(); super.dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/disco/lib/src/widgets/provider_scope.dart(9 hunks)
🔇 Additional comments (17)
packages/disco/lib/src/widgets/provider_scope.dart (17)
31-36: Clear documentation of the ordering requirement.The documentation clearly explains the ordering constraint and references the appropriate error type. This will help developers understand the feature and avoid forward-reference errors.
63-143: Excellent centralization of provider access logic.The
_getOrCreateValuehelper elegantly handles bothProviderandArgProvideraccess with proper forward-reference detection. The two-step approach (check initialization scope first, then search ancestors) is well-structured and maintainable.The ordering validation correctly prevents both forward references and self-references by checking
requestedIndex >= currentIndex.
159-168: Clean refactoring to use the centralized helper.Delegating to
_getOrCreateValueeliminates duplication and ensures consistent behavior across both provider types.
191-201: Clean refactoring to use the centralized helper.Delegating to
_getOrCreateValueeliminates duplication and ensures consistent behavior across both provider types.
223-237: Well-designed tracking fields for initialization.The combination of static scope tracking and instance-level index tracking enables robust same-scope dependency resolution while preventing circular references. The comments clearly explain each field's purpose.
243-256: Proper initialization with cleanup guarantees.The try-finally block ensures that
_currentlyInitializingScopeand_currentlyCreatingProviderIndexare always cleared, even if initialization throws. This prevents stale state from affecting subsequent operations.
258-293: Thorough uniqueness validation.The separate validation loops for
ProviderandArgProvidermake the logic clear and maintainable. The assertions appropriately catch configuration errors during development.
295-325: PHASE 1: Well-structured provider registration.Registering all providers before creating any is essential for same-scope access to work correctly. The separation of concerns between registration and creation makes the initialization flow easy to follow.
327-356: PHASE 2: Correct non-lazy provider creation.Setting and clearing
_currentlyCreatingProviderIndexfor each provider ensures accurate forward-reference detection. The sequential creation in declaration order is essential for the same-scope dependency feature.
358-363: Clear initialization orchestration.The three-step initialization flow is well-organized and easy to understand.
429-439: Clear override initialization orchestration.Separating provider and arg provider override processing keeps the logic modular and maintainable.
365-394: Robust provider override processing.The uniqueness validation and immediate value creation for overrides is correct. The renamed error class (
MultipleProviderOverrideOfSameInstance) is more specific than the old name.
396-427: Robust arg provider override processing.Consistent with the provider override logic. Proper validation and immediate creation of override values.
501-524: Correct same-scope support for lazy arg providers.Consistent with
createProviderValue. The save/restore pattern properly handles nested lazy provider creation.
673-687: Improved error with dynamic naming.Using the provider's
debugNamein error messages significantly improves debuggability. The switch expression cleanly handles bothProviderandArgProvidertypes.
704-716: Improved error class naming.The shortened name
MultipleProviderOverrideOfSameInstanceis clearer and more concise while maintaining the same meaning. Appropriate for the v2 major version bump.
718-774: Excellent error class with actionable guidance.The error message is exceptionally clear and helpful:
- Explains the forward reference with specific indices
- Describes why the constraint exists (prevent circular dependencies)
- Provides concrete fix instructions (reorder the providers)
This will significantly improve the developer experience when encountering ordering issues.
Deploying flutter-disco with
|
| Latest commit: |
51ad98f
|
| Status: | 🚫 Build failed. |
nank1ro
force-pushed
the
feat/access-providers-in-same-scope
branch
from
70a8946 to
6d02e04
Compare
|
@copilot Do you think the new implementation can be improved by avoiding for loops and using maps only? |
Co-authored-by: nank1ro <60045235+nank1ro@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Provider Benchmark ResultsDate: 2025-12-15 11:13:54 UTC Results
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/content/docs/miscellaneous/reactivity.md (1)
12-46: Example wiring is consistent with same-scope semantics; fix a small syntax issue and consider showing debugNameThe move to a single
ProviderScopewith:providers: [counterProvider, doubleCounterProvider],is aligned with the new “declare dependent providers after their dependencies” rule and matches how
doubleCounterProviderdepends oncounterProvider.Two small suggestions:
- Missing semicolon in the example
final doubleCounterProvider = Provider((context) { return Computed(() => counterProvider.of(context).value * 2) });should have a semicolon after the
Computed(...)call:- return Computed(() => counterProvider.of(context).value * 2) + return Computed(() => counterProvider.of(context).value * 2);
- Optional: showcase
debugNameSince this PR introduces
debugName, you might consider using it here to help users understand error messages:final counterProvider = Provider( (context) => Signal(0), debugName: 'counterProvider', ); final doubleCounterProvider = Provider( (context) => Computed(() => counterProvider.of(context).value * 2), debugName: 'doubleCounterProvider', );
🧹 Nitpick comments (11)
packages/disco/benchmark/README.md (2)
73-74: Confusing wording: dependencies vs dependents.Line 74 states "Order providers with dependencies after their dependents" but this seems inverted. A provider with dependencies should be declared after the providers it depends on (its dependencies), not after its dependents.
Consider rewording:
-3. **Order providers** with dependencies after their dependents +3. **Order providers** with dependencies after the providers they depend on
149-151: Minor: Capitalize "Markdown" as a proper noun.1. Runs all benchmark tests -2. Generates a markdown table with results +2. Generates a Markdown table with results 3. Posts results as a PR comment (for pull requests)packages/disco/PERFORMANCE_SUMMARY.md (1)
40-40: Fragile line number reference.Referencing specific line numbers in documentation (
Lines 254-277) will quickly become outdated as the codebase evolves. Consider removing the line number reference or linking to a specific commit/tag.-### 1. Optimized Duplicate Detection (Lines 254-277) +### 1. Optimized Duplicate Detectionpackages/disco/benchmark/provider_benchmark.dart (1)
461-476: Verification creates a new ProviderScope, not validating the benchmarked one.The verification step pumps a completely new widget tree with a fresh
ProviderScope, which creates new provider instances rather than verifying the values from the benchmarked scope. This works because the providers are deterministic, but the comment "Verify the final value is correct" is somewhat misleading—it's verifying the logic is correct, not that the benchmarked providers retained their values.Consider adding a clarifying comment or restructuring to verify within the same widget tree:
// Verify the final value is correct + // Note: This creates a new scope to verify provider logic, not the benchmarked instance await tester.pumpWidget(.github/workflows/benchmark.yaml (2)
68-71: Bot comment detection could match unintended comments.The condition
comment.user.type === 'Bot' && comment.body.includes('Provider Benchmark Results')might match comments from other bots (e.g., CodeRabbit, Dependabot) if they happen to include "Provider Benchmark Results" in their body. Consider using a more specific marker.- const botComment = comments.data.find(comment => - comment.user.type === 'Bot' && - comment.body.includes('Provider Benchmark Results') + const BENCHMARK_MARKER = '<!-- disco-benchmark-results -->'; + const botComment = comments.data.find(comment => + comment.user.type === 'Bot' && + comment.body.includes(BENCHMARK_MARKER) );Then update
_writeBenchmarkResults()in the benchmark file to include the marker at the start of the output.
53-59: Consider adding error handling for missing results file.If the benchmark tests fail before writing
benchmark_results.md, the workflow will fail at the "Comment PR with results" step. Consider adding a file existence check.- name: Comment PR with results if: github.event_name == 'pull_request' uses: actions/github-script@v7 with: script: | const fs = require('fs'); + const resultsPath = 'packages/disco/benchmark_results.md'; + if (!fs.existsSync(resultsPath)) { + console.log('Benchmark results file not found, skipping PR comment'); + return; + } - const results = fs.readFileSync('packages/disco/benchmark_results.md', 'utf8'); + const results = fs.readFileSync(resultsPath, 'utf8');packages/disco/OPTIMIZATION_NOTES.md (1)
163-163: Same terminology confusion as in README."Place providers with dependencies AFTER their dependents" should likely read "AFTER their dependencies" (the providers they depend on).
-1. **Order matters**: Place providers with dependencies AFTER their dependents (validated in debug mode) +1. **Order matters**: Place providers with dependencies AFTER the providers they depend on (validated in debug mode)packages/disco/lib/src/widgets/provider_scope.dart (3)
31-35: Clarify wording in provider ordering docsThe semantics are clear, but the sentence could be slightly polished: “Providers that depend on other providers…” (plural verb) reads better than “depends”. Also consider explicitly saying “in the same
providerslist of thisProviderScope” to avoid ambiguity.
373-402: Align error type for ArgProvider overrides with provider overrides
_processProviderOverridesthrowsMultipleProviderOverrideOfSameInstance, while_processArgProviderOverridesthrowsMultipleProviderOfSameInstanceon duplicate overrides. That’s slightly confusing, since both code paths are dealing with overrides.For consistency of diagnostics, consider using the override-specific error in both places:
- if (ids.contains(id)) { - throw MultipleProviderOfSameInstance(); - } + if (ids.contains(id)) { + throw MultipleProviderOverrideOfSameInstance(); + }(and adjust the doc comment of
MultipleProviderOverrideOfSameInstanceif you want it to explicitly mention arg providers as well).Also applies to: 404-447
696-706: ProviderWithoutScopeError fallback branch could be more robustSwitching on
providerand using_debugNamefor bothProviderandArgProvidermakes the error much more readable. The default branch currently throws anException, though, which meanstoString()itself can fail if a new provider type is ever passed in.Consider falling back to
provider.runtimeType.toString()instead of throwing, so logging this error never introduces a second error:- _ => throw Exception('Unknown provider type ${provider.runtimeType}'), + _ => provider.runtimeType.toString(),This keeps diagnostics resilient even if new provider flavours are introduced later.
docs/src/content/docs/core/scoped-di.mdx (1)
68-79: Docs accurately describe same-scope ordering; keep wording and behaviour alignedThe updated “Full example” and “Scoping correctly with context” sections clearly explain that:
- Providers can depend on providers from ancestor scopes, and
- Within the same
ProviderScope, a dependent provider must be declared after the provider it depends on, otherwise aProviderForwardReferenceErroris thrown.The “Wrong” vs “Correct” examples for
doubleNumberPlusArgProvidervsnumberProviderillustrate this nicely.Two minor tweaks:
- Grammar/capitalisation
- “Some providers might have a dependency on other providers, to handle such cases, you can:” → consider splitting into two sentences:
“Some providers might depend on other providers. To handle such cases, you can:”- “to make it work, just swap the order of declaration:” → capitalise “To”.
- Keep docs in sync with runtime behaviour
Right now the docs state that a
ProviderForwardReferenceError“will be thrown at runtime” when the order is wrong. In the implementation, that check is currently guarded bykDebugMode, so in release builds it won’t fire. Once you adjust the code so forward-reference detection runs in all build modes (see earlier comment inprovider_scope.dart), this wording will be accurate; otherwise, you may want to qualify this as “in debug mode” to avoid confusion.Also applies to: 86-114
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
.github/workflows/benchmark.yaml(1 hunks)docs/src/content/docs/core/scoped-di.mdx(2 hunks)docs/src/content/docs/miscellaneous/reactivity.md(1 hunks)packages/disco/OPTIMIZATION_NOTES.md(1 hunks)packages/disco/PERFORMANCE_SUMMARY.md(1 hunks)packages/disco/benchmark/README.md(1 hunks)packages/disco/benchmark/provider_benchmark.dart(1 hunks)packages/disco/lib/src/widgets/provider_scope.dart(12 hunks)
🧰 Additional context used
🪛 LanguageTool
packages/disco/OPTIMIZATION_NOTES.md
[style] ~196-~196: It might be better to use ‘time’ with the time-relative pronoun ‘when’. (Alternatively, use ‘in/on which’.)
Context: ...: Deep scope nesting causes O(d) lookup time where d is depth Current: Each scope look...
(WHEN_WHERE)
packages/disco/benchmark/README.md
[uncategorized] ~150-~150: Did you mean the formatting language “Markdown” (= proper noun)?
Context: ...Runs all benchmark tests 2. Generates a markdown table with results 3. Posts results as ...
(MARKDOWN_NNP)
🔇 Additional comments (10)
packages/disco/PERFORMANCE_SUMMARY.md (1)
1-132: LGTM!The performance summary provides a clear and well-organized explanation of the optimizations made. The before/after comparisons with complexity analysis are helpful for understanding the changes.
packages/disco/benchmark/provider_benchmark.dart (3)
560-603: LGTM!The
_writeBenchmarkResultsfunction is well-structured, generates a clear Markdown report with proper ordering, and handles missing results gracefully with'N/A'.
1-54: LGTM!Good use of
Stopwatchfor timing, proper test structure withtestWidgets, and clear benchmark naming conventions. The debug mode disclaimer in the generated report is appropriate.
519-556: The test is correctly structured. Reusing the sameproviderslist across nested scopes does not trigger duplicate provider detection—each scope validates its own provider list independently, and since the list contains 20 unique Provider instances with no duplicates, validation passes for all scopes. This approach is valid for benchmarking nested scope overhead.Likely an incorrect or invalid review comment.
.github/workflows/benchmark.yaml (1)
1-45: LGTM!The workflow is well-structured with appropriate triggers, caching for Flutter, and correct working directory configuration. The path filters ensure benchmarks only run when relevant code changes.
packages/disco/OPTIMIZATION_NOTES.md (2)
276-320: Good risk assessment on Provider Pooling.The documentation correctly identifies the dangers of provider pooling (breaks scope isolation, testing complexity, lifecycle issues) and marks it as high-risk. This honest assessment helps future maintainers understand why this optimization was not pursued.
1-425: LGTM!Excellent technical documentation. The three-phase initialization explanation, complexity analysis, and future optimization proposals with honest trade-off assessments provide valuable context for maintainers. The section on parallel initialization correctly identifies Flutter's synchronous widget constraints.
packages/disco/lib/src/widgets/provider_scope.dart (3)
263-285: Uniqueness check forbids multiple InstantiableArgProviders for the same ArgProvider
_validateProvidersUniquenessde-duplicatesInstantiableArgProviderentries byitem._argProvider, not by the instantiable itself. This means aproviderslist like:ProviderScope( providers: [ doubleNumberPlusArgProvider(10), doubleNumberPlusArgProvider(20), ], child: ... )will throw
MultipleProviderOfSameInstancein debug builds, even though the two entries differ by argument.Is this intentional? If you want to support scoping the same
ArgProviderwith multiple arguments in a single scope, this assertion should distinguish instances (or include the argument) rather than the shared_argProvider. If you intend to forbid that pattern, it might be worth calling it out explicitly in the docs.
565-573: Coverage ignores around debug-only diagnostics are fineMarking
debugFillProperties,updateShouldNotify, andupdateShouldNotifyDependentwith// coverage:ignore-*is reasonable here, since they’re deterministic and not critical to behavioural correctness. No changes needed.Also applies to: 582-587, 603-611
727-739: New override and forward-reference error types look good
MultipleProviderOverrideOfSameInstanceandProviderForwardReferenceErrorare well named, and theProviderForwardReferenceErrormessage is clear about what went wrong and how to fix it (move$requestedNamebefore$currentNamein the list).Once the forward-reference detection is made to run in release as well (see earlier comment), these errors should be very helpful for diagnosing mis-ordered providers.
Also applies to: 741-786
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/disco/test/disco_test.dart (1)
811-827: Test description still references old error nameThe test description still says “MultipleProviderOverrideOfSameProviderInstance” while the expectation now uses
MultipleProviderOverrideOfSameInstance. Functionally the test is correct, but the wording is slightly stale.You can align the description with the new type name:
- testWidgets( - '''ProviderScopeOverride must throw a MultipleProviderOverrideOfSameProviderInstance for duplicated providers''', + testWidgets( + '''ProviderScopeOverride must throw a MultipleProviderOverrideOfSameInstance for duplicated providers''', (tester) async {
🧹 Nitpick comments (4)
.github/workflows/benchmark.yaml (1)
52-68: README update logic assumes Benchmark is the final section.The sed command (line 61) removes everything from "## Benchmark" to the end of file. If other sections are added after the Benchmark section in the future, they will be accidentally deleted. Consider a more defensive approach.
For robustness, replace the sed-based deletion with explicit section replacement:
- name: Update README with benchmark results if: github.event_name == 'push' && (github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev') run: | # Read benchmark results BENCHMARK_CONTENT=$(cat packages/disco/benchmark_results.md) # Check if README already has a Benchmark section if grep -q "^## Benchmark" packages/disco/README.md; then # Remove everything from ## Benchmark to the end - sed -i '/^## Benchmark/,$d' packages/disco/README.md + # More robust: extract everything before ## Benchmark + sed -i '/^## Benchmark/,$d' packages/disco/README.md fi # Add benchmark section at the end echo "" >> packages/disco/README.md echo "## Benchmark" >> packages/disco/README.md echo "" >> packages/disco/README.md echo "$BENCHMARK_CONTENT" >> packages/disco/README.mdAlternatively, use a marker-based approach if other sections may follow Benchmark:
sed -i '/^## Benchmark/,/^## [^B]/d; /^## Benchmark/d' packages/disco/README.mdpackages/disco/lib/src/widgets/provider_scope.dart (3)
63-133: Same‑scope creation + forward‑reference detection logic looks correct; note behaviour for overridesThe combination of:
_getOrCreateValue’s same‑scope branch,_currentlyInitializingScope/_currentlyCreatingProviderIndex/_currentlyCreatingProvider,- index maps populated in
_registerAllProviders, and- the wrapping in
createProviderValue/createProviderValueForArgProvidergives a coherent model:
- Forward references in the same
ProviderScope(requested index ≥ current index) reliably throwProviderForwardReferenceErrorin both init and lazy creation.- Earlier providers (including lazy ones) in the same scope are created on demand and reused via
createdProviderValues, with static state correctly saved/restored through nested calls and cross‑scope access.- Cross‑scope reads skip the same‑scope branch via
isInScopeand fall back to the usual ancestor/portal lookup.One nuance: overrides initialized via
_initializeOverridesdon’t populate_providerIndices/_argProviderIndicesor set_currentlyCreatingProviderIndex, so forward‑reference detection and “same‑scope” semantics effectively do not apply within override lists. That’s perfectly safe, but it’s an implicit difference fromproviders; if you ever want overrides to participate in the same ordering rules, you’d need a similar “phase 1 register + phase 2 create with index tracking” flow there too.Also applies to: 213-253, 279-351, 450-518
255-277: Duplicate‑provider checks are assert‑only; confirm that dev‑only enforcement is desired
_validateProvidersUniqueness,_processProviderOverrides, and_processArgProviderOverridesall throwMultipleProviderOfSameInstance/MultipleProviderOverrideOfSameInstanceonly from insideassertblocks. This mirrors the existing pattern, but means:
- In debug/tests: duplicates throw as expected.
- In release: duplicates are silently allowed, and later entries just overwrite earlier ones in the maps.
If the intent is to treat duplicates as programmer errors that must never reach production, the current design is fine. If you want consistent runtime behaviour between debug and release, consider moving the checks out of
assertor duplicating them in non‑assert code.Also applies to: 353-427
659-681: Error type refinements and messages align with new semantics
ProviderWithoutScopeErrornow takes a genericObject providerand derives a debug name for bothProviderandArgProvider, which matches the updated tests and makes messages clearer.MultipleProviderOverrideOfSameInstancecleanly distinguishes override duplication from base provider duplication.ProviderForwardReferenceErrorcarries bothcurrentProviderandrequestedProviderand formats them via_debugName, producing a concise, actionable error that aligns with the declaration‑order rules.The defensive
switchfallbacks that throw for unknown provider types should be unreachable given the current API surface; if you later introduce new provider‐like types, you might want to either broaden the pattern or fall back toruntimeTypeinstead of throwing fromtoString, but as written this is acceptable for an “impossible” branch.Also applies to: 698-710, 712-757
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/benchmark.yaml(1 hunks)packages/disco/.pubignore(1 hunks)packages/disco/benchmark/provider_benchmark.dart(1 hunks)packages/disco/lib/src/widgets/provider_scope.dart(12 hunks)packages/disco/test/disco_test.dart(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/disco/.pubignore
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/disco/benchmark/provider_benchmark.dart
🔇 Additional comments (6)
.github/workflows/benchmark.yaml (1)
83-119: GitHub Script for PR comments is well-structured.The logic correctly detects existing bot comments by checking for both Bot user type and the comment text identifier. Updating vs. creating comments prevents duplication and keeps the PR clean.
packages/disco/lib/src/widgets/provider_scope.dart (2)
31-36: Doc comment clearly communicates ordering contractThe added documentation on
providersexplicitly states the declaration-order constraint and theProviderForwardReferenceErrorbehaviour; this matches the implementation and will help users avoid subtle bugs.
536-544: Coverage ignores around debug‑only InheritedWidget hooks are reasonableMarking
debugFillProperties,updateShouldNotify, andupdateShouldNotifyDependentwith coverage ignores keeps noise out of coverage reporting while still exercising the main provider logic elsewhere. No functional concerns here.Also applies to: 553-583
packages/disco/test/disco_test.dart (3)
171-195: Updated ArgProvider “not found” test matches new error APIThe test now correctly expects
ProviderWithoutScopeErrorand inspects the unified.providerfield, asserting it equals theArgProviderinstance. This aligns with the refactored error type and ensures ArgProviders are covered by the same error path as regular Providers.
517-571: Portal + same‑scope lazy provider test exercises the new common pathThe
ProviderScopePortal with lazy providers accessing same‑scope providerstest nicely validates that:
- Lazy providers declared in the same
ProviderScopecan depend on each other via declaration order, and- Access through
ProviderScopePortalstill hits the same_getOrCreateValuepath (including caching and same‑scope semantics).This is a good regression test for the portal + same‑scope integration.
859-1305: New same‑scope, forward‑reference, caching, and circular‑dependency tests provide strong coverageThe added block of tests around:
- same‑scope access for combinations of lazy/non‑lazy Providers and ArgProviders,
- forward references in both directions (Provider→Provider, ArgProvider→Provider, Provider→ArgProvider, ArgProvider→ArgProvider),
- reuse of previously created values when multiple dependents access the same earlier provider, and
- “impossible by design” circular dependencies that surface as
ProviderForwardReferenceErrorgives very thorough coverage of the new ordering semantics. The scenarios mirror the implementation details (index maps, initialization vs lazy creation, mixed Provider/ArgProvider lists) and should catch regressions in
_getOrCreateValueor the index bookkeeping.No functional issues spotted; this suite looks well designed for the new behaviour.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/disco/test/disco_test.dart (1)
1189-1221: Later ArgProvider forward-reference test now matches error typeThe test name and expectation both target
ProviderForwardReferenceError, addressing the previously noted mismatch between description and asserted type.
🧹 Nitpick comments (2)
packages/disco/test/disco_test.dart (2)
517-571: Portal + lazy same-scope test exercises the right code pathsThis test nicely covers
ProviderScopePortalwith lazy providers that depend on an earlier same-scope provider, ensuring the portal path and cached-value resolution behave as expected. One minor readability nit: consider renaming the localdoublevariable to something likedoubleValueto avoid shadowing thedoubletype.
859-1187: Strong coverage of same-scope ordering, laziness, ArgProviders, and cachingThis block of tests thoroughly exercises the new semantics: non-lazy vs lazy combinations, Provider↔ArgProvider interactions, multi-step chains (A→B→C), and reuse of a single created value when multiple providers read from the same earlier provider. It should give good regression protection around same-scope access and caching behaviour. If you ever want to reduce duplication, many of these
MaterialApp/Scaffold/ProviderScope+Builderscaffolds could be factored into a small helper, but that’s optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/src/content/docs/miscellaneous/reactivity.md(1 hunks)packages/disco/test/disco_test.dart(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
docs/src/content/docs/miscellaneous/reactivity.md (1)
16-25: Example now matches same-scope provider semanticsRegistering both
counterProvideranddoubleCounterProviderin a singleProviderScopein dependency order, along with the fixedComputedreturn line, correctly illustrates the new same-scope access model and forward-reference behaviour.packages/disco/test/disco_test.dart (3)
171-195: ArgProvider not-found test now matches unified ProviderWithoutScopeError APIThe description,
TypeMatcher<ProviderWithoutScopeError>, and use oferror.providerare consistent and aligned with the non-Arg provider case, which keeps the error surface coherent.
811-827: Override duplication test correctly targets MultipleProviderOverrideOfSameInstanceThe test description and expectation now both use
MultipleProviderOverrideOfSameInstance, clearly separating override-duplication failures fromMultipleProviderOfSameInstancein regular scopes.
1223-1305: Circular-dependency prevention tests clearly encode the forward-reference invariantBoth scenarios (Provider→Provider and Provider→ArgProvider) demonstrate that any would-be cycle in a single scope must hit a forward reference first and thus raise
ProviderForwardReferenceError, which captures the intended design constraint and helps guard against regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/disco_lint/lib/src/assists/base/wrap_builder.dart (1)
1-8: Implementation imports from internal analyzer APIs create maintenance risk.The code imports three internal APIs from analyzer's
/src/paths (lines 5-7):
analyzer/src/dart/ast/extensions.dartanalyzer/src/dart/element/type.dartanalyzer/src/utilities/extensions/flutter.dartWhile the
analyzer: ^8.0.0constraint in pubspec.yaml prevents major version breakage, it does not protect against breaking changes in minor versions. The analyzer package is known to make breaking changes within minor versions without notice, creating fragility.Add inline comments explaining why each implementation import is necessary and what public analyzer or analyzer_plugin API would serve as an alternative if available. Consider pinning analyzer more tightly (e.g.,
^8.1.0or8.1.x) if targeting specific known-stable versions.
🧹 Nitpick comments (1)
packages/disco/analysis_options.yaml (1)
14-16: Reconsider suppressing documentation warnings for a major version release.Adding
document_ignores: ignoresilences all documentation-related analyzer warnings globally. For a major version bump (v2) that introduces significant API changes (debugName parameter, provider ordering semantics), this risks masking real documentation gaps that should be addressed—especially for public APIs that consumers depend on.If specific files legitimately lack documentation by design, prefer using file-level ignore comments (e.g.,
// ignore: document_ignoresor/// ignore: document_ignores) rather than project-wide suppression.Please clarify:
- Why is this suppression necessary for the v2 release?
- Are there intentionally undocumented public APIs, or should these be documented instead?
- If suppression is needed, would file-level ignores be more appropriate than project-wide?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
examples/solidart/analysis_options.yaml(1 hunks)packages/disco/CHANGELOG.md(1 hunks)packages/disco/analysis_options.yaml(1 hunks)packages/disco/pubspec.yaml(1 hunks)packages/disco/test/disco_test.dart(17 hunks)packages/disco_lint/example/pubspec.yaml(1 hunks)packages/disco_lint/lib/src/assists/base/wrap_builder.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/disco/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/disco/pubspec.yaml
- examples/solidart/analysis_options.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Flutter test
- GitHub Check: Run Provider Benchmarks
🔇 Additional comments (8)
packages/disco_lint/example/pubspec.yaml (1)
14-15: LGTM!The switch to a local path dependency aligns with the workspace setup and is the appropriate pattern for an example project during development. This allows the example to test against the latest local disco package changes.
packages/disco/test/disco_test.dart (6)
9-9: LGTM! Appropriate lint suppression.The
unreachable_from_mainignore is correct sinceNameContaineris only used in test code.
167-172: LGTM! Error type and field updates are consistent.The renamed error types (
ProviderWithoutScopeError,MultipleProviderOfSameInstance) and unified field access (.provider) are correctly applied throughout the test suite, matching the PR's public API changes.Also applies to: 175-201, 203-228
385-385: LGTM! Proper future handling in tests.Using
.ignore()on unawaitedshowDialogfutures is the correct approach to suppress theunawaited_futureslint in test scenarios where the dialog result is not needed.Also applies to: 441-441, 515-515, 573-573
885-1251: Excellent comprehensive test coverage for same-scope provider access!This test suite thoroughly validates the new same-scope provider dependency feature:
- Tests all combinations of lazy/non-lazy providers
- Correctly validates forward-reference detection
- Verifies value caching when multiple providers access the same dependency
- Tests nested dependency chains (A→B→C)
- Covers both
ProviderandArgProviderscenariosThe test logic and expectations are correct throughout. This provides strong confidence in the feature's correctness.
1253-1339: Excellent demonstration of circular dependency prevention by design!These tests effectively validate that the declaration-order enforcement makes circular dependencies impossible within the same scope. Any attempt to create circular dependencies results in
ProviderForwardReferenceError, which aligns perfectly with the PR's stated goal of "preventing circular dependencies by design."The test comments clearly explain the prevention mechanism, making this a valuable documentation reference.
536-589: LGTM! Important edge case coverage.This test validates that same-scope provider dependencies work correctly through
ProviderScopePortal, including proper context navigation and cached value retrieval. This is an important validation of the feature's robustness across different widget tree patterns.packages/disco_lint/lib/src/assists/base/wrap_builder.dart (1)
10-84: LGTM!The WrapBuilder abstraction is well-designed with proper null safety, clean separation of concerns, and correct indentation handling for generated code. The use of early returns keeps the logic clear, and the builder pattern integration with analyzer_plugin APIs is implemented correctly.
|
I'll have time to review it this week. Probably tomorrow or on Thursday ;) |
manuel-plavsic
left a comment
manuel-plavsic
left a comment
There was a problem hiding this comment.
I am still not finishing reviewing. I still need to review the main part of the provider_scope.dart file. I will do it ASAP.
I like the idea of a user-friendlier API 👍
| The provider `doubleNumberPlusArgProvider` depends on `numberProvider`. Therefore, when scoping them both in the same `ProviderScope`, `numberProvider` must be declared first. | ||
|
|
There was a problem hiding this comment.
after "... must be declared first." Add: "Therefore, the following code will lead to a ProviderWithoutScopeError:"
and then you enter the wrong example.
| ``` | ||
|
|
||
| Placing the `ProviderScope` containing `doubleNumberPlusArgProvider` above the one containing `numberProvider` would also not work. It needs to be like in the full example above. | ||
| to make it work, just swap the order of declaration: |
There was a problem hiding this comment.
I would change this line to : "The error can be prevented by reordering the list of providers:"
| analyzer: | ||
| errors: | ||
| always_put_required_named_parameters_first: ignore | ||
| specify_nonobvious_property_types: ignore |
There was a problem hiding this comment.
Good catch! Disco is very neat, most of the time the types will be inferred correctly ;).
The only reason to specify a different type is when working with sub-classes, but in that case one would have to do it anyway.
Maybe we can recommend setting this rule somewhere in the docs too?
| @@ -1,5 +1,7 @@ | |||
| ## 2.0.0 | |||
|
|
|||
| - **FEAT**: Allow providers declared in the same `ProviderScope` to depend on each other, by leveraging the order of declaration. This simplifies the development experience and prevents circular dependencies by design. | |||
There was a problem hiding this comment.
Two things:
- "to depend on each other" this is wrong. We are just letting the ones defined below to depend on the ones defined above.
- Circular dependencies were actually never a thing. Also, the screenshot you uploaded in the PR does not feature a circular dependency, but the prevention mechanism.
So, I think this point should be reframed to:
"Allow providers in the same ProviderScope to depend on previously declared providers. This simplifies the development experience. This friendlier syntax does not introduce circular dependencies."
| analyzer: | ||
| errors: | ||
| document_ignores: ignore |
There was a problem hiding this comment.
I don't see the point of adding this? Can you should me an example of its usefulness?
| /// | ||
| /// Providers can be either [Provider] or [ArgProvider]. | ||
| /// Providers that depends on other providers in the same [providers] list | ||
| /// must be listed after the providers they depend on, otherwise a | ||
| /// [ProviderForwardReferenceError] will be thrown. | ||
| final List<InstantiableProvider>? providers; |
There was a problem hiding this comment.
Is this the right place for these comments?
Also, an InstantiableProvider can be either a Provider or an InstantiableArgProvider (i.e. an ArgProvider with its argument). The comment says it can be either a Provider or an ArgProvider. It is wrong and misleading.
I overall think the comments might just add confusion. I would remove them and keep the same docstring as before.
|
|
||
| /// Track the scope currently being initialized. This enables same-scope | ||
| /// provider access during initialization. | ||
| static ProviderScopeState? _currentlyInitializingScope; |
There was a problem hiding this comment.
I guess this can be solved like this... I am just a bit unsure. Are we basically using global state for the current scope?
| /// Creates a provider value and stores it to [createdProviderValues]. | ||
| dynamic createProviderValue(Provider id) { | ||
| dynamic createProviderValue(Provider id, BuildContext context) { | ||
| // find the intermediate provider in the list | ||
| final provider = getIntermediateProvider(id)!; | ||
| // create and return its value | ||
| final value = provider._createValue(context); | ||
| // store the created provider value | ||
| createdProviderValues[id] = value; | ||
| return value; | ||
|
|
||
| // Support same-scope access for lazy providers | ||
| final savedScope = _currentlyInitializingScope; | ||
| final savedIndex = _currentlyCreatingProviderIndex; | ||
| final savedProvider = _currentlyCreatingProvider; | ||
| try { | ||
| _currentlyInitializingScope = this; | ||
| _currentlyCreatingProviderIndex = _providerIndices[id]; | ||
| _currentlyCreatingProvider = id; | ||
|
|
||
| // create and return its value | ||
| final value = provider._createValue(context); | ||
| // store the created provider value | ||
| createdProviderValues[id] = value; | ||
| return value; | ||
| } finally { | ||
| _currentlyInitializingScope = savedScope; | ||
| _currentlyCreatingProviderIndex = savedIndex; | ||
| _currentlyCreatingProvider = savedProvider; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add some comments here, such as:
/// Creates a provider value and stores it to [createdProviderValues].
dynamic createProviderValueForArgProvider(
ArgProvider id,
BuildContext context,
) {
// Find the intermediate provider in the list
final provider = getIntermediateProviderForArgProvider(id)!;
// Temporarily override shared creation state
final savedScope = _currentlyInitializingScope;
final savedIndex = _currentlyCreatingProviderIndex;
final savedProvider = _currentlyCreatingProvider;
try {
_currentlyInitializingScope = this;
_currentlyCreatingProviderIndex = _argProviderIndices[id];
_currentlyCreatingProvider = id;
// Create the provider value (may throw or trigger nested creation)
final value = provider._createValue(context);
// Store the created provider value
createdProviderValues[allArgProvidersInScope[id]!] = value;
return value; // finally still runs after this return
} finally {
// Restore shared state on both success and failure
_currentlyInitializingScope = savedScope;
_currentlyCreatingProviderIndex = savedIndex;
_currentlyCreatingProvider = savedProvider;
}
}
| /// Creates a provider value and stores it to [createdProviderValues]. | ||
| dynamic createProviderValueForArgProvider( | ||
| ArgProvider id, | ||
| BuildContext context, | ||
| ) { | ||
| // find the intermediate provider in the list | ||
| final provider = getIntermediateProviderForArgProvider(id)!; | ||
| // create and return its value | ||
| final value = provider._createValue(context); | ||
| // store the created provider value | ||
| createdProviderValues[allArgProvidersInScope[id]!] = value; | ||
| return value; | ||
|
|
||
| // Support same-scope access for lazy providers | ||
| final savedScope = _currentlyInitializingScope; | ||
| final savedIndex = _currentlyCreatingProviderIndex; | ||
| final savedProvider = _currentlyCreatingProvider; | ||
| try { | ||
| _currentlyInitializingScope = this; | ||
| _currentlyCreatingProviderIndex = _argProviderIndices[id]; | ||
| _currentlyCreatingProvider = id; | ||
|
|
||
| // create and return its value | ||
| final value = provider._createValue(context); | ||
| // store the created provider value | ||
| createdProviderValues[allArgProvidersInScope[id]!] = value; | ||
| return value; | ||
| } finally { | ||
| _currentlyInitializingScope = savedScope; | ||
| _currentlyCreatingProviderIndex = savedIndex; | ||
| _currentlyCreatingProvider = savedProvider; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add some comments here too
manuel-plavsic
left a comment
manuel-plavsic
left a comment
There was a problem hiding this comment.
There are a few things to adjust, bit it LGTM otherwise. The core logic seems sound. Good job!
| /// access during initialization and lazy creation. | ||
| static T? _getOrCreateValue<T extends Object, ID extends Object>({ | ||
| required BuildContext context, | ||
| required ID id, |
There was a problem hiding this comment.
Write that ID is either Provider<T> or ArgProvider<T, A> in the function's docstring. Makes the code immediately more understandable.
| /// Map each provider to its index in the original providers list. | ||
| /// Used to enforce ordering constraints during same-scope access. | ||
| final HashMap<Provider, int> _providerIndices = HashMap<Provider, int>(); | ||
|
|
||
| /// Map each ArgProvider to its index in the original providers list. | ||
| /// Used to enforce ordering constraints during same-scope access. | ||
| final HashMap<ArgProvider, int> _argProviderIndices = | ||
| HashMap<ArgProvider, int>(); |
There was a problem hiding this comment.
We can omit the type here thanks to type inference.
/// Map each provider to its index in the original providers list.
/// Used to enforce ordering constraints during same-scope access.
final _providerIndices = HashMap<Provider, int>();
/// Map each ArgProvider to its index in the original providers list.
/// Used to enforce ordering constraints during same-scope access.
final _argProviderIndices = HashMap<ArgProvider, int>();| try { | ||
| if (widget.providers != null) { | ||
| _initializeProviders(widget.providers!); | ||
| } else if (widget.overrides != null) { | ||
| _initializeOverrides(widget.overrides!); | ||
| } |
There was a problem hiding this comment.
I am a bit rusty with our code base. Is it correct that we give precedence to providers over overrides here?
| final provider = | ||
| instantiableArgProvider._argProvider._generateIntermediateProvider( | ||
| instantiableArgProvider._arg, | ||
| ); | ||
| allArgProvidersInScope[id] = provider; |
There was a problem hiding this comment.
I know this is some old code, but VSCode is formatting it differently.
What is going on? Can you format the file and tell me if the code looks like in this PR or like in my screenshot?
There was a problem hiding this comment.
not only here, but throughout the entire code base
| // coverage:ignore-start | ||
| if (createdProvider != null) return createdProvider as T; | ||
| // coverage:ignore-end |
There was a problem hiding this comment.
Also here, VSCode formats it like this in my case:
It could also be that I am doing something wrong.
|
|
||
| /// Helper method to handle common logic between Provider and ArgProvider | ||
| /// access during initialization and lazy creation. | ||
| static T? _getOrCreateValue<T extends Object, ID extends Object>({ |
There was a problem hiding this comment.
This is the core function of this PR. I inspected it, and it seems fine to me. It uses global state to track the current instantiating ProviderScope, but it seems to be pretty safe IMO, since there can only be one instantiating ProviderScope at a time thanks to the event loop. I don't see a better or more efficient alternative here. Good job!
ProviderScopeto depend on each other, by leveraging the order of declaration. This simplifies the development experience and prevents circular dependencies by design.debugNameparameter to providers for easier debugging, allowing better identification of providers in error messages and logs.This now works:
while by reversing the order of the providers declaration (
doubleCounterProviderbeforecounterProvider) you get this runtime error:This eliminates the possibility to create a circular dependency.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.