3182/migrate maps to mobx#2218
Open
iobuhov wants to merge 20 commits into
Open
Conversation
- Replace useLocationResolver hook with MobX-based LocationResolver service - Implement SetupComponent pattern for lifecycle management - Add proper async handling with observable state + reaction() - Use makeObservable with annotations instead of decorators - Add 20 comprehensive test cases covering: * Basic functionality (coordinates, geocoding) * Mixed markers and edge cases * API key handling and caching * Error recovery * MobX reactivity * Static + dynamic markers integration - Integrate @mendix/widget-plugin-test-utils for proper mocks - Achieve 100% coverage on model layer - Follow Gallery widget architecture pattern Test results: 42/42 passing, 88.69% overall coverage
- Replace all setTimeout/Promise delays with MobX when() helper - Mock convertAddressToLatLng at module level for deterministic tests - Add waitForLocations() helper using when() for observable changes - Configure MobX with enforceActions: 'never' for testing - Add timeout handling for error cases - Improve test reliability and speed (3.5s vs 13s) - All 42 tests still passing with 100% model layer coverage
Split 598-line test file into 3 self-contained files: - LocationResolver.unit.spec.ts (247 lines) - Basic functionality, empty inputs, API keys - LocationResolver.integration.spec.ts (321 lines) - Mixed markers, caching, errors, integration - LocationResolver.reactivity.spec.ts (226 lines) - MobX reactions and observable behavior Benefits: - Each file self-contained with inline setup (no helpers folder) - Follows Gallery/Datagrid patterns in the repo - Easy to locate specific test types - Can run test files independently - No abstraction layers to learn - Clear test intent without jumping to other files All 45 tests passing with 100% model layer coverage maintained
Add 26 unit tests for convertStaticModeledMarker and convertDynamicModeledMarker: convertStaticModeledMarker (5 tests): - All fields present - Undefined optional fields - Number parsing with comma/period decimal separators - Custom marker image handling convertDynamicModeledMarker (21 tests): - Datasource availability (undefined, Loading, Unavailable, empty) - Coordinates location type (single/multiple markers, missing attributes) - Address location type (with/without address attribute) - Optional fields (title, onClick action, custom marker) - Edge cases (item IDs, NaN handling, empty strings, mixed attributes) Test results: - 26/26 tests passing - 100% code coverage on data.ts - Self-contained tests using @mendix/widget-plugin-test-utils - Uses list(), obj(), listAttribute(), listAction(), dynamic() helpers
Remove test 'should handle NaN from invalid coordinate strings' because: - Dynamic markers use ListAttributeValue<Big>, not string attributes - Mendix runtime ensures type safety - we never receive invalid coordinate types - The scenario being tested doesn't occur in practice Tests: 70 passed (was 71) Coverage: Still 100% on data.ts
Remove 'should handle multiple markers with different attributes' test because: - Already covered by 'should convert multiple markers with coordinates' test - Doesn't add unique value - tests same scenario with different attribute values - Simplifies test suite without losing coverage Tests: 69 passed (was 70) Coverage: Still 100% on data.ts
Improve markers() computed getter: - Remove mutable array and push operations - Store static/dynamic markers in separate variables - Use .flat() instead of .reduce() for flattening - Return immutable spread [...staticMarkers, ...dynamicMarkers] Benefits: - More functional style (no mutations) - Clearer intent with named variables - Modern Array.flat() is more readable than reduce - Shorter and easier to understand Before: const markers = []; markers.push(...static); const flattened = dynamic.reduce((prev, curr) => [...prev, ...curr], []); markers.push(...flattened); After: const staticMarkers = ... const dynamicMarkers = ....flat(); return [...staticMarkers, ...dynamicMarkers]; All tests passing (69/69)
Move marker equality comparison from manual check to reaction equals option: Before: - Manual isIdenticalMarkers() check inside effect - Separate private method for comparison - Early return if markers unchanged After: - Built-in equals option in reaction config - MobX handles comparison before effect runs - Cleaner effect logic without manual check Benefits: - More idiomatic MobX (comparison at framework level) - Simpler effect code (no manual equality check) - Remove isIdenticalMarkers() method (less code) - Same behavior: still compares markers excluding action callbacks The equals function: - Maps prev/next to remove action field - Uses deepEqual with strict mode - Returns true if markers unchanged (prevents effect run) All tests passing (69/69)
Move API key retrieval from inline expression to computed getter: Before: - Inline in reaction effect - const apiKey = this.mainGate.props.geodecodeApiKeyExp?.value ?? this.mainGate.props.geodecodeApiKey; After: - Computed getter property - get apiKey(): string | undefined - Use this.apiKey in reaction Benefits: - Cleaner reaction code (one less variable) - Better MobX reactivity tracking - Reusable if needed elsewhere - Clear documentation via JSDoc - Follows computed pattern for derived values All tests passing (69/69)
Three improvements to LocationResolverService: 1. Version counter instead of reference equality: - Replace requestedMarkers reference check - Use geocodeVersion number (++version pattern) - Clearer intent: 'is this the latest request?' - Minimal memory (number vs full array) 2. Remove runInAction wrapper: - updateLocations is already an action - No need for extra runInAction wrapper - Simpler, cleaner code 3. Geocode function as dependency: - Extract GeocodeFunction type in tokens - Inject via constructor (better testability) - Bind in RootContainer (shared utility) - Remove direct import of convertAddressToLatLng Benefits: - Better dependency injection pattern - Easier to mock in tests - Clearer async handling with version counter - Less memory usage - Follows DI best practices All tests passing (69/69)
Move dependency injection registration from service to container: Before: - Service file had: injected(LocationResolverService, ...) - Container had: injected() call in inject() method - Duplication of DI setup After: - Service file: Clean, no DI registration - Container file: Single source of DI registration - Removed unused imports (injected, CORE_TOKENS) Benefits: - DI setup in one place (container) - Service file is cleaner - Follows separation of concerns - Container owns binding configuration The injected() call now lives only in Maps.container.ts where the binding happens, not scattered in service files. All tests passing (69/69)
Fix 'should update props when they change' test to verify correct behavior: Before: - Checked if config.name updated (it doesn't - config is static) - Test was incomplete and didn't verify actual prop changes After: - Check mainGate.props.name (correct source of reactive props) - Verify props actually update via the gate - Test now validates the real behavior Key insight: - Config is static (bound once at container creation) - MainGate provides reactive access to current props - Services use mainGate.props for up-to-date values The test now correctly verifies that: 1. Container instance remains stable (reusability) 2. MainGate provides updated props (reactivity) All tests passing (69/69)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…ests - Create test-utils.ts with createTestContainer helper that uses real MapsContainer - Replace jest.mock() with explicit dependency injection via container - Override geocodeFunction binding before container initialization - Properly trigger setup lifecycle to start MobX reactions - All 22 LocationResolver tests passing with improved type safety and maintainability Benefits: - Tests now use real container setup, catching integration issues - Explicit, type-safe mocking of dependencies - Reusable test utilities for other service tests - 100% coverage of LocationResolverService
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
1 participant
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.
Pull request type
Description