fix: use keys from across all rows to construct csv object #2478
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2478 +/- ##
===========================================
- Coverage 83.05% 52.77% -30.29%
===========================================
Files 925 925
Lines 20612 20610 -2
Branches 3256 3254 -2
===========================================
- Hits 17120 10876 -6244
- Misses 3373 9319 +5946
- Partials 119 415 +296
... and 299 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
projects/components/src/download-file/service/file-download.service.test.ts
Outdated
Show resolved
Hide resolved
| const allKeys = getAllDataKeys(data); | ||
| const allKeysDict = allKeys.reduce((acc, key) => ({ ...acc, [key]: undefined }), {}); | ||
|
|
||
| return data.map(row => ({ ...allKeysDict, ...row })); |
There was a problem hiding this comment.
Because we're using object.values to iterate this below, we're still relying here on insertion order being consistent, which it won't be if we're spreading like this (also IIRC there's some nuances to the iteration order of object key/values, so best not to rely on it). suggest we iterate on keys to access values rather than using them as-is to overwrite empty values like this.
There was a problem hiding this comment.
Are you suggesting we do this instead?:
data.map((row) => {
const newRow = Object.fromEntries(allKeys.map((key) => [key, row?.[key] ?? undefined]));
return newRow;
})There was a problem hiding this comment.
there's multiple approaches here, but I probably would avoid mapping it into an object and back out again. Not only is it extra allocations, but more important - you're relying on semantics (both of .fromEntries and of .values). Instead, map allKeys to the stringify calls.
| // If given header is empty, then create header from the data keys | ||
| const header$ = isEmpty(config.header) | ||
| ? config.dataSource.pipe( | ||
| map(data => Object.keys(data[0] ?? [])), | ||
| map(keys => keys.map(startCase)) | ||
| map( |
There was a problem hiding this comment.
give there's a decent amount of logic here, can we also write a real test?
This comment has been minimized.
This comment has been minimized.
| const values$: Observable<string[][]> = config.dataSource.pipe( | ||
| // All rows are not guaranteed to have all keys, so add all keys to all rows | ||
| map(data => { | ||
| const allKeys = getAllDataKeys(data); |
There was a problem hiding this comment.
nit: now that I read this a little closer, we're being pretty inefficient on doing this. We're iterating all the values to calculate the keys twice, iterating to calculate the values once and then iterating again to merge them. If you think that makes it more readable, by all means go for it. Personally, I think it does the opposite - I'd try to pipe off the data source a single time.
More importantly though, I see the header can be specified via config. If that's the case, we still have an issue. If I give a config of "header1, header2" and data containing extra keys (or keys in a different order) both the prior code and our new code would produce mismatched rows. However we calculate the header, we need to use that same result to extract the data.
There was a problem hiding this comment.
Updated: 1a62ad4 (#2478)
| // If given header is empty, then create header from the data keys | ||
| const header$ = isEmpty(config.header) | ||
| ? config.dataSource.pipe( | ||
| map(data => Object.keys(data[0] ?? [])), | ||
| map(keys => keys.map(startCase)) | ||
| map( |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Unit Test Results 4 files 316 suites 47m 6s ⏱️ Results for commit 3170394. |
| ) | ||
| : of(config.header!); | ||
|
|
||
| const getAllDataKeys = (data: Dictionary<unknown>[]): string[] => uniq(data.map(row => Object.keys(row)).flat()); |
There was a problem hiding this comment.
nit - now we only do this once, don't need to declare it as a separate function.
| // Value replacer for null and undefined values | ||
| const replacer = (_: string, value: string) => value ?? '-'; | ||
| const csvData$ = config.dataSource.pipe( | ||
| map(data => { | ||
| const allKeys = isEmpty(config.header) ? getAllDataKeys(data) : config.header!; |
There was a problem hiding this comment.
One major concern here is there's a big assumption that the header provided is keys, and not already cased/renamed. Do we know that to be true for all use cases? If it is, I wouldn't even call it the header any more since that sounds like it should not impact the extraction. Would call it like csvColumn with a key and optional title or something.
Description
CSV download logic should look at all keys across the row list, not just first row.