Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable Export with Excel Format #214

Merged
merged 12 commits into from
May 21, 2024
5 changes: 5 additions & 0 deletions .changeset/silly-months-unite.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'contexture-export': minor
---

Enable Excel Export Option
4 changes: 3 additions & 1 deletion packages/export/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
"contexture-client": "^2.53.7",
"futil": "^1.76.4",
"lodash": "^4.17.21",
"minimal-csv-formatter": "^1.0.15"
"minimal-csv-formatter": "^1.0.15",
"read-excel-file": "^5.8.1",
positiveVibes4all marked this conversation as resolved.
Show resolved Hide resolved
"write-excel-file": "^2.0.1"
},
"packageManager": "yarn@3.3.1"
}
4 changes: 3 additions & 1 deletion packages/export/src/csv.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ export default ({
// and key of the record.
// [{ key: string(supports lodash dot notation), label: string, display: function(value, {key, record, transform})}...]
transform,
headers = null, // array of strings to use as headers, array or arrays for multi-line headers
//TODO: support multi-line headers in excel and csv when implemented
headers = null, // array of strings to use as headers
onWrite = _.noop, // function to intercept writing a page of records
}) => {
stream.write(csv(headers || transformLabels(transform)))
Expand Down Expand Up @@ -39,6 +40,7 @@ export default ({
recordsWritten = recordsWritten + _.getOr(1, 'recordCount', r)
await onWrite({ recordsWritten, record: r })
}
await onWrite({ recordsWritten: recordsWritten, isStreamDone: true })
positiveVibes4all marked this conversation as resolved.
Show resolved Hide resolved
await stream.end()
})(),
cancel() {
Expand Down
78 changes: 78 additions & 0 deletions packages/export/src/excel.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you write tests for this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What type of test would you suggest? the data is binary, so we really cannot build one like is done for CSV's, I thought about it but it felt like there was little value add for doing this based on this constraint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can read the stream via https://www.npmjs.com/package/read-excel-file, which is nice as you don't need to write a file in the test, keeping it pure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems ok, I am not in love with it because we are using another library to test this one in essence but seems like a start.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's by the same author so I think that reduces the risk somewhat

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it just feels like we are testing the library itself instead of anything novel about how it is used, I will add it though as it is at least something. Thanks for the feedback on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking is that there is some code that can benefit from testing which is not library related. Another approach is to take a writeData callback that looks like

      const readStream = await writeXlsxFile(excelData, { columns })
      for await (const chunk of readStream) {
        stream.write(chunk)
      }

That way you don't have to read the xlsx file but can still test this function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminder that this still needs to be done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in the current PR and the tests is using it as such.

Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
import _ from 'lodash/fp.js'
import writeXlsxFile from 'write-excel-file/node'

const convertToExcelCell = (value) => {
return {
type: String,
positiveVibes4all marked this conversation as resolved.
Show resolved Hide resolved
typeof: 'string',
positiveVibes4all marked this conversation as resolved.
Show resolved Hide resolved
wrap: true,
value: `${value}`,
}
}

export const writeStreamData = async (stream, readStream) => {
for await (const chunk of readStream) {
stream.write(chunk)
}
}

let transformLabels = _.map(_.get('label'))

export default ({
stream, // writable stream target stream
iterableData, // iterator for each page of an array of objects
// order list of which indicates the header label,
// display function for the field,
// and key of the record.
// [{ key: string(supports lodash dot notation), label: string, display: function(value, {key, record, transform})}...]
transform,
//TODO: support multi-line headers in excel and csv when implemented
headers = null, // array of strings to use as headers
onWrite = _.noop, // function to intercept writing a page of records
}) => {
const excelData = [
_.map(
(value) => ({ value, fontWeight: 'bold' }),
headers || transformLabels(transform)
),
]
stellarhoof marked this conversation as resolved.
Show resolved Hide resolved

let cancel = false
let recordsWritten = 0

return {
promise: (async () => {
for await (let r of iterableData) {
if (cancel) break
excelData.push(
_.map(
(t) =>
convertToExcelCell(
t.display(_.get(t.key, r), {
key: t.key,
record: r,
transform,
})
),
transform
)
)
recordsWritten = recordsWritten + _.getOr(1, 'recordCount', r)
await onWrite({ recordsWritten })
}
let columns = _.map(
() => ({
width: 50,
Copy link
Member

@dubiousdavid dubiousdavid May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the default behavior for width? If the default behavior is not smart, we should consider setting the width to the largest value of the following: the length of the column name or 50.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default is not smart, 50 seemed like a reasonable size to start with and wrapping the data set to true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote some code to keep track of the longest value and then use the column name as the shortest

}),
excelData[1]
)
const readStream = await writeXlsxFile(excelData, { columns })
await writeStreamData(stream, readStream)
stellarhoof marked this conversation as resolved.
Show resolved Hide resolved
await onWrite({ recordsWritten: recordsWritten, isStreamDone: true })
positiveVibes4all marked this conversation as resolved.
Show resolved Hide resolved
await stream.end()
})(),
cancel() {
cancel = true
},
}
}
66 changes: 66 additions & 0 deletions packages/export/src/excel.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import _ from 'lodash/fp.js'

Check failure on line 1 in packages/export/src/excel.test.js

View workflow job for this annotation

GitHub Actions / ESLint

packages/export/src/excel.test.js#L1

'_' is defined but never used (no-unused-vars)
import { PassThrough } from 'stream'
import writeXlsxFile from 'write-excel-file/node'
import readXlsxFile from 'read-excel-file/node'
import { writeStreamData } from './excel.js'

const testDate = new Date()

const HEADER_ROW = [
{
value: 'Name',
fontWeight: 'bold',
},
{
value: 'Date of Birth',
fontWeight: 'bold',
},
{
value: 'Cost',
fontWeight: 'bold',
},
{
value: 'Paid',
fontWeight: 'bold',
},
]

const DATA_ROW_1 = [
{
type: String,
value: 'John Smith',
},
{
type: Date,
value: testDate,
format: 'mm/dd/yyyy',
},
{
type: Number,
value: 1800,
},
{
type: Boolean,
value: true,
},
]

const expectedFileContents = [
['Name', 'Date of Birth', 'Cost', 'Paid'],
['John Smith', testDate, 1800, true],
]

const excelData = [HEADER_ROW, DATA_ROW_1]

// These are skipped on purpose as they actual write CSVs
describe('Excel Stream Test', () => {
it('Export to Excel and Validate', async () => {
const writeStream = new PassThrough()
const readStream = await writeXlsxFile(excelData)
await writeStreamData(writeStream, readStream)
const rows = await readXlsxFile(writeStream)
console.log(rows)
console.log(expectedFileContents)
expect(rows).toStrictEqual(expectedFileContents)
})
})
3 changes: 2 additions & 1 deletion packages/export/src/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import csv from './csv.js'
import excel from './excel.js'
import * as nodes from './nodes/index.js'

export { nodes, csv }
export { nodes, csv, excel }
export * from './utils.js'
Loading
Loading