Repository navigation
add sheetjs adaptor - #1790
Open
mtuchi wants to merge 8 commits into
Open
add sheetjs adaptor#1790mtuchi wants to merge 8 commits into
mtuchi wants to merge 8 commits into
Conversation
mtuchi
marked this pull request as ready for review
September 22, 2026 07:21
mtuchi
force-pushed
the
sheetjs-adaptor
branch
from
September 22, 2026 10:01
7f7266c to
a25c98b
Compare
hunterachieng
left a comment
hunterachieng
left a comment
Contributor
There was a problem hiding this comment.
A single-column CSV can be silently misread as base64 and return an empty array with no error. Example: sheetToJson("name\nJohn\nJane") returns []. The stripped content "nameJohnJane" (12 chars) passes the base64 round-trip check, so it's decoded as binary garbage instead of text. This is data-dependent ("id\n1234\n5678" works fine), so it'll pass in testing and corrupt real files in production. The JSDoc at Adaptor.js:94 acknowledges the ambiguity but there's no guard and no test. Can we bias detection toward text when the content contains newlines, and add a regression test?
Operations previously accepted `null` to reuse a workbook cached in a
module-level closure. That made the data flow implicit, and reserved a
positional argument for "look at hidden state".
Each of parse, sheetToJson and sheetToCsv now takes the file content and
parses it, so jobs reference the file from state as usual:
sheetToJson($.data.fileContent, { sheetName: 'Orders' })
A parsed workbook is deliberately never written to state. expandReferences
rebuilds objects from their own keys, and a Date has none, so a workbook
routed through state comes back with every date nulled. Buffers and strings
pass through untouched, so content is safe to reference with `$.`.
The trade is that reading several sheets from one file parses it several
times - roughly 470ms per call on a 10MB file, a few ms on a typical one.
Also removes the setWorkbook test seam (and its build warning), tightens the
JSDoc descriptions that become the public docs, and reworks the date
regression test around a lazy state reference.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Export the full SheetJS library as `xlsx` for use in job code, and document it in the README - Restrict file content to a Buffer or a string (base64, base64 data URL or raw csv text). Uint8Array/ArrayBuffer never worked, since expandReferences walks them as plain objects, so drop them from the docs and throw an error suggesting Buffer.from() instead - Test that a Uint8Array is rejected with that hint Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
- Pass dateNF to xlsx.read in sheetToJson and sheetToCsv. SheetJS formats cell text at read time and sheet_to_csv/sheet_to_json ignore dateNF for cells that already have text, so the option had no effect - Move the cellDates: false example from parse to sheetToJson, where it changes the output, and document cellDates on SheetToJsonOptions - Reject a data URL with no comma, and validate base64 with decodeBase64 before falling back to a plain decode - Test sheetToCsv RS, strip, blankrows, rawNumbers and dateNF; sheetToJson cellDates and dateNF; parse with cellDates: false; and jsonToSheet with output: 'base64' and bookType: 'csv' - Remove redundant comments Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
mtuchi
force-pushed
the
sheetjs-adaptor
branch
from
October 1, 2026 15:30
58fa6d0 to
a3d7e62
Compare
Collaborator
Author
|
Hey @hunterachieng i have addressed your feedback can you take a look again |
hunterachieng
left a comment
hunterachieng
left a comment
Contributor
There was a problem hiding this comment.
@mtuchi I think some comments were missed. Do you mind checking those out?
Rename describe to describeType and handle undefined items, simplify isWrappedLikeBase64, and assert exact error messages in tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Hey @hunterachieng can you give this another round of review ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a
sheetjsadaptor for excel transformationFixes #1718
Details
Added the following functions
jsonToSheet, Build a spreadsheet file from an array of objectssheetToCsv, Convert a worksheet into a CSV stringsheetToJson, Convert a worksheet into an array of objects,parse, Parse spreadsheet file content into a workbook.xlsx, To use all xlsx utils functionsAI Usage
Please disclose how you've used AI in this work (it's cool, we just want to
know!):
You can read more details in our
Responsible AI Policy
Review Checklist
Before merging, the reviewer should check the following items:
production? Is it safe to release?
dev only changes don't need a changeset.
pnpm run versionused to bump versions (notpnpm changeset versiondirectly)? This ensures changelog dates are stamped correctly.