SALTO-7272: Use calculateDiff instead of getPlan in createDiffChanges API
#7191
Conversation
|
also, a bit of PR etiquette - fill in the PR description before asking for a review. |
Nurdok
force-pushed
the
feature/SALTO-7272-createDiffChanges
branch
2 times, most recently
from
ad8dbf2 to
5fed5a1
Compare
packages/core/src/core/diff.ts
Outdated
| .toArray() | ||
| } | ||
|
|
||
| export async function createDiffChangesWithGetPlan({ |
There was a problem hiding this comment.
Shouldn't be exported
| resultType, | ||
| }) | ||
| } | ||
| return createDiffChangesWithGetPlan({ |
There was a problem hiding this comment.
nit: Probably worth adding a TODO here with a SALTO ticket to delete this
packages/core/src/core/diff.ts
Outdated
| resultType: 'changes' | ||
| }): Promise<ChangeWithDetails[]> | ||
| export function createDiffChanges(params: { | ||
| export async function createDiffChangesWithCalculateDiff({ |
There was a problem hiding this comment.
same, shouldn't be exported
packages/core/src/core/diff.ts
Outdated
| export async function createDiffChanges({ | ||
| resultType?: 'changes' | 'detailedChanges' | ||
| }): Promise<DetailedChangeWithBaseChange[] | ChangeWithDetails[]> { | ||
| if (elementSelectors.length > 0) { |
There was a problem hiding this comment.
I know this logic has been here before, but I'm thinking maybe we can consolidate this for better readability ?
If we map all changes into { ...change, detailedChanges: () => filteredDetailedChanges }
then, we create a filter function that only if there are matchers will filter out the detailed changes (or the entire change), other wise will always return true
Then we can avoid some of the branching here and the repetition starting in line 163
WDYT?
Nurdok
force-pushed
the
feature/SALTO-7272-createDiffChanges
branch
from
5fed5a1 to
2875987
Compare
| ? awu(changes) | ||
| .map(change => { | ||
| const filteredDetailedChanges = getDetailedChangesFromChange(change, compareOptions).filter( | ||
| matchers.isChangeMatchSelectors, |
There was a problem hiding this comment.
In most cases this function will be a constant "return true" function (from here), in which case, calculating the detailed changes from the change is a waste of cycles.
it could be a noticeable improvement to avoid calculating detailed changes here.
# Conflicts: # packages/core/src/core/fetch.ts
packages/core/src/core/diff.ts
Outdated
| before: toElementsSrc, | ||
| after: fromElementsSrc, | ||
| topLevelFilters: | ||
| elementSelectors.length > 0 ? topLevelFilters.concat(matchers.isTopLevelElementMatchSelectors) : topLevelFilters, |
There was a problem hiding this comment.
nit - consolidate elementSelectors.length > 0 and elementSelectors.length === 0 to a single const ?
Use
calculateDiffinstead ofgetPlanincreateDiffChangesAPI. This behavior is guarded by a core flag. The API already returned the computed diff as a list of changes, making this quite a short diff.Additional context for reviewer:
This PR is stacked on top of #7187 which introduces the core flag used here.
Release Notes:
None
User Notifications:
None