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

fix(read): allow reading base64 files from a dataURI scheme #2763

Merged
merged 3 commits into from
Aug 8, 2022

Conversation

AbdelrahmanHafez
Copy link
Contributor

fixes #2762

I'm not if the modified files are source files that get compiled or those are the compiled files.
Also, can you point me to where should the test cases for those be?

it('handles base64 within data URI scheme (gh-2762), function() {
  // Arrange
  const fileInBase64 = 'TmFtZXMNCkhhZmV6DQpTYW0NCg==';
  const fileInBase64WithDataURIScheme = 'data:text/csv;base64,TmFtZXMNCkhhZmV6DQpTYW0NCg==';

  // Act
  const workBookFromRawBase64 = XLSX.read(fileInBase64, { type: 'base64' });
  const workBookFromBase64WithinDataURI = XLSX.read(fileInBase64WithDataURIScheme, { type: 'base64' });

  // Assert
  assert.deepStrictEqual(workBookFromRawBase64, workBookFromBase64WithinDataURI);
});

@SheetJSDev
Copy link
Contributor

The test should be added between line 690 and 691 in test.js: https://github.com/SheetJS/sheetjs/blob/master/test.js#L690-L691 (just after the "should read base64 strings" test)

The media type is optional and the comma need not be escaped. Try /^data:([^\/]+\/[^\/]+)?;base64,/

Include a test with a string like

  var fileInBase64WithDataURISchemeNoMediaType = 'data:;base64,TmFtZXMNCkhhZmV6DQpTYW0NCg==';

The test snippet should use avoid const and any other ES6+ features.

@AbdelrahmanHafez
Copy link
Contributor Author

@SheetJSDev
Thanks a lot for the detailed comment, I've pushed the necessary changes.

@SheetJSDev SheetJSDev merged commit 6bea47a into SheetJS:master Aug 8, 2022
@AbdelrahmanHafez AbdelrahmanHafez deleted the gh-2762 branch August 8, 2022 20:52
AbdelrahmanHafez added a commit to AbdelrahmanHafez/sheetjs that referenced this pull request Aug 8, 2022
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.

[Improvement] reading excel file from base64 should allow ignoring data uri information
2 participants