refactor(*): further improvements to core and grids/core#16742
refactor(*): further improvements to core and grids/core#16742
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the core and grids/core modules by reorganizing grid navigation constants and the yieldingLoop utility function to improve code organization and reduce coupling between the core and grids packages.
Changes:
- Moved grid-specific navigation key constants from
core/utils.tstogrids/core/grid-navigation-keys.ts - Moved
yieldingLoopfunction fromcore/utils.tstogrids/core/exporter-common/yielding-loop.ts - Updated all import statements across affected files to reference the new locations
- Removed unused
setImmediate.tsfile from core package - Updated
zone.jsdependency from~0.15.0to~0.16.0
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pivot-grid-navigation.service.ts | Updated imports to use grid-specific navigation keys from grids/core |
| hierarchical-grid-navigation.service.ts | Updated imports to use SUPPORTED_KEYS from grids/core |
| public_api.ts (grid) | Added export for grid-group-by-area component |
| group-by-area.component.html | New file containing template for group-by-area component |
| grid-group-by-area.component.ts | Updated imports to consolidate from grids/core |
| grid.pipes.ts | Changed let to const for immutable result variable |
| grid.component.ts | Moved IgxGridGroupByAreaComponent import to local file |
| grid-base.directive.ts | Removed IgxGridGroupByAreaComponent from grids/core import, added local import |
| yielding-loop.ts | New file containing extracted yieldingLoop utility function |
| base-export-service.ts | Updated import to use local yieldingLoop |
| excel-files.ts | Updated import to use local yieldingLoop |
| char-separated-value-data.ts | Updated import to use local yieldingLoop |
| public_api.ts (grids/core) | Added export for grid-navigation-keys, removed grid-group-by-area component export |
| grid-navigation.service.ts | Updated imports to use grid-navigation-keys module |
| grid-navigation-keys.ts | New file containing grid-specific navigation key constants |
| grid-mrl-navigation.service.ts | Updated imports to use grid-navigation-keys module |
| utils.ts (core) | Removed grid-specific constants and yieldingLoop function, removed setImmediate import |
| setImmediate.ts | Deleted file as it's no longer used |
| accordion.component.ts | Moved ACCORDION_NAVIGATION_KEYS constant to local definition |
| package.json | Updated zone.js dependency version |
| callback(i); | ||
| } | ||
| if (i < count) { | ||
| setTimeout(chunk, 0); |
There was a problem hiding this comment.
The original implementation used setImmediate for scheduling, which provides better performance characteristics than setTimeout(0) by using the microtask queue. Consider documenting why the change to setTimeout was made, or evaluate if this change impacts export performance for large datasets.
There was a problem hiding this comment.
@gedinakova Can you look at this comment and if it correctly reflects a performance impact, I will ask it to revert it back to setImmediate.
There was a problem hiding this comment.
Based on the unit test I tried the two within, the setImmediate definitely gives a way better performance:
Chrome Headless 143.0.0.0 (Windows 10) LOG LOG: 'setTimeout(0) completed in: 986.60ms'
Chrome Headless 143.0.0.0 (Windows 10) LOG LOG: 'setImmediate completed in: 2.40ms'.
I vote for going back to setImmediate.
|
@copilot Based on the following comment: |
…ldingLoop Co-authored-by: kdinev <1472513+kdinev@users.noreply.github.com>
Co-authored-by: kdinev <1472513+kdinev@users.noreply.github.com>
refactor(grids): restore setImmediate for export performance
|
@copilot Add a migration for the moved export of the |
…to grids/grid Co-authored-by: kdinev <1472513+kdinev@users.noreply.github.com>
[WIP] Refactor core and grids/core for further improvements
Closes #
Additional information (check all that apply):
Checklist:
feature/README.MDupdates for the feature docsREADME.MDCHANGELOG.MDupdates for newly added functionalityng updatemigrations for the breaking changes (migrations guidelines)