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

Local data storage and Excel upload/download #27

Merged
merged 7 commits into from
Jul 17, 2024
Merged

Conversation

kwheelan
Copy link
Contributor

@kwheelan kwheelan commented Jul 16, 2024

Closes #23. Related to #20.

@kwheelan kwheelan marked this pull request as draft July 16, 2024 19:19
@kwheelan kwheelan marked this pull request as ready for review July 16, 2024 20:33
@kwheelan
Copy link
Contributor Author

@maxatdetroit If you have a chance, could you look at the way I'm handling and storing the Excel data and give me your thoughts? The relevant functions are in js/utils/data_utils/ (but I can move them if you think there's a better spot in terms of code organization).

Basically, I'm reading in the Excel file, saving it in local storage in a JSON-like format, editing that copy with any user changes, and then using the same data to write back to an Excel file available for download at the end. Local storage doesn't seem perfect for this, and the initial Excel file read-in is a bit slow, but I think it might be the best (only?) way to do it with a pure front-end solution. I've been using the Excel file in sample_data/ to test.

Copy link
Member

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

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

The code generally looks good given the XLSX/JSON approach.

I left a note about how we might resolve the performance issue if you think the performance is worth taking a deeper look at.

giphy

workbook.SheetNames.forEach(sheetName => {
// only convert sheets we need
if (Object.keys(SHEETS).includes(sheetName)) {
// read in sheets
const sheet = workbook.Sheets[sheetName];
const rawData = XLSX.utils.sheet_to_json(sheet, { header: 1, defval: null });
const rawData = XLSX.utils.sheet_to_json(sheet, { header: 1, defval: '' });
Copy link
Member

Choose a reason for hiding this comment

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

the initial Excel file read-in is a bit slow

It's hard to say if it's the arraybuffer read / Blob write or JSON serialization / deserialization that is slowing things down without profiling the code. How bad is the performance? How does it scale with the size of the file / data?

Local storage doesn't seem perfect for this, ... , but I think it might be the best (only?) way to do it with a pure front-end solution

If the performance hit is in the JSON serialization / deserialization, then there is the option to use IndexedDB which is a client-side NoSQL style storage. Then we could store the data as a binary blob instead of JSON. That would have a performance improvement but require some extra effort to setup.

Is the performance hit worth addressing in your opinion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not that slow -- a couple seconds. Based on a quick look into IndexedDB, it's probably not worth changing the approach for now, but I'll test the current code to see how long it takes to process some of the larger departments.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I agree. A couple seconds isn't bad. A better, lower-effort fix would probably be to handle the IO and serialization operations as asynchronous operations and provide some UX treatment like a spinner while they process.

@@ -81,6 +81,6 @@ export function lastPage(){
}

export async function pauseAndContinue(){
Copy link
Member

Choose a reason for hiding this comment

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

Why do we pause execution before proceeding in the app when the user clicks the next button?

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 was trying to smooth the transition between pages, but it doesn't really look as intended, so I'll delete.

@kwheelan kwheelan merged commit 7038bc8 into dev Jul 17, 2024
@kwheelan kwheelan deleted the 23-excel-data-handling branch July 17, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants