Repository navigation
Support the endings option in the Blob and File constructors - #8461
littledivy wants to merge 1 commit into
Conversation
`endings` was `jsg::Unimplemented` on `BlobPropertyBag` and `FilePropertyBag`, so `new Blob(parts, { endings })` threw for any value, even the valid `'transparent'` and `'native'`. Code that sets `endings: 'native'` when it builds a text file couldn't run.
In this patch:
- `endings` is parsed as the File API's `EndingType` enum. `'transparent'` (the default) keeps the bytes as they are. Any other value, such as `null`, `''` or `'NATIVE'`, throws a `TypeError`, as in browsers.
- With `'native'`, each string part has its CRLF and lone CR converted to the native line ending before it is encoded. Buffer and Blob parts are left as they are, and each string is converted on its own, so `['\r', '\n']` gives two line endings.
- The native line ending is LF on every platform, including workerd on Windows. Workers run on Linux, so a Blob built in local development has the same bytes as one built when deployed.
- The WPT `Blob-constructor-endings` and `File-constructor-endings` tests now pass. Conversion tests are in `blob-test.js`, since WPT only checks which values are accepted.
| | before | after |
|---|---|---|
| `new Blob(['a\r\nb'], { endings: 'native' })` | throws `Error` | `'a\nb'` |
| `new Blob(['a\r\nb'], { endings: 'transparent' })` | throws `Error` | `'a\r\nb'` |
| `new Blob([], { endings: 'NATIVE' })` | throws `Error` | throws `TypeError` |
The option threw for every value before, so no working code changes behaviour, and it ships without a compatibility flag. Leaving `endings` out gives the same bytes as before.
There was a problem hiding this comment.
✅ Approved — I found nothing to fix: the endings parsing and the CRLF/CR-to-LF conversion are correct, the new tests cover them, and the change is safe without a compatibility flag because the option always threw before.
Not opened by the correctness review: 3 changed files it was meant to read
types/generated-snapshot/experimental/index.tstypes/generated-snapshot/index.d.tstypes/generated-snapshot/index.ts
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
No issues found. The output buffer is exactly input length minus the CRLF count, so the conversion cannot write past the end.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found. The line-ending conversion sizes its output correctly, leaves strings without a CR untouched, and runs before the part sizes are cached and the blob size limit is checked.
⚡ Performance: ✅ LGTM
No issues found. A string with no CR is returned without allocating, and conversion only happens when endings: 'native' is set.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: 09a8115d
endingswasjsg::UnimplementedonBlobPropertyBagandFilePropertyBag, sonew Blob(parts, { endings })threw for any value, even the valid'transparent'and'native'. Code that setsendings: 'native'when it builds a text file couldn't run.In this patch:
endingsis parsed as the File API'sEndingTypeenum.'transparent'(the default) keeps the bytes as they are. Any other value, such asnull,''or'NATIVE', throws aTypeError, as in browsers.'native', each string part has its CRLF and lone CR converted to the native line ending before it is encoded. Buffer and Blob parts are left as they are, and each string is converted on its own, so['\r', '\n']gives two line endings.Blob-constructor-endingsandFile-constructor-endingstests now pass. Conversion tests are inblob-test.js, since WPT only checks which values are accepted.new Blob(['a\r\nb'], { endings: 'native' })Error'a\nb'new Blob(['a\r\nb'], { endings: 'transparent' })Error'a\r\nb'new Blob([], { endings: 'NATIVE' })ErrorTypeErrorThe option threw for every value before, so no working code changes behaviour, and it ships without a compatibility flag. Leaving
endingsout gives the same bytes as before.