Skip to content

bug fix can not read property date1904 of undefined #1328

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

Merged
merged 2 commits into from
Jun 28, 2020

Conversation

mouse9
Copy link

@mouse9 mouse9 commented Jun 15, 2020

Summary

Issue:
ExcelJs.stream.xlsx.WorkbookReader reading with styles: cache option was giving
Cannot read property 'date1904' of undefined

Reproduce with this code

const workbookReader = new excel.stream.xlsx.WorkbookReader(fileReadStream, { worksheets: 'emit', styles: 'cache', sharedStrings: 'cache', hyperlinks: 'ignore', entries: 'ignore' });
    workbookReader.read();
    workbookReader.on('worksheet', worksheet => { 
         worksheet.on('row', (row) => {...}
     }

Above code will throw an error when DD/MM/YYYY format date is given in the excel
e.g: 20/06/2020

I found that in lib/stream/xlsx/worksheet-reader.js we are using it but not checking for the undefined model case.

Copy link
Member

@Siemienik Siemienik left a comment

Choose a reason for hiding this comment

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

Looks good for me, thanks

I have only one request - it's required to cover each detected (and fixed) bug by a test. Could I ask you to write at least one?

@mouse9
Copy link
Author

mouse9 commented Jun 18, 2020

Yeah sure.
I understand, I was just waiting for your reply first.

I will add test cases now

@mouse9 mouse9 mentioned this pull request Jun 23, 2020
@mouse9
Copy link
Author

mouse9 commented Jun 23, 2020

@Siemienik I have added the integration test case for the same :)

@Siemienik Siemienik merged commit b7df1c5 into exceljs:master Jun 28, 2020
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.

4 participants