Strip a leading BOM from source

[tylerturdenpants]

content-tag strips a leading byte order mark before parsing, so every byte offset it reports is relative to the stripped source. coordinatesOf and ParseResultStringUtils both sliced the caller's original source by those offsets, and that source still had the BOM, so the whole window landed three bytes to the left.

Found while adopting 0.7.0 in stylelint-ember-scoped-css, where .gts files are linted and a Windows-authored component has to keep working.

coordinatesOf returned indices into the wrong text

const source = "const X = 1;\n<template>\n  <style scoped>\n...";

coordinatesOf(source, parsed[0]);
// { line: 2, column: 10, columnOffset: 0, start: 23, end: 74 }   correct

coordinatesOf("" + source, parsedWithBom[0]);
// { line: 2, column:  7, columnOffset: 0, start: 21, end: 72 }   start should be 24

transformSync silently did nothing

This one is the reason I bothered, since nothing errors:

const source = "export const Foo = <template>Hello</template>\n";

transformSync(source, (c) => c.replace("Hello", "Goodbye"));
// "export const Foo = <template>Goodbye</template>\n"

transformSync("" + source, (c) => c.replace("Hello", "Goodbye"));
// "export const Foo = <template>Hello</template>\n"   unchanged

The callback got a shifted window ("te>Hello th" rather than "Hello"), so the replacement found nothing to match and the file came back byte-identical.

The change

Per @NullVoxPopuli's review, a BOM isn't part of the source anymore, rather than being something to offset around. Every entry point strips it on the way in, nothing hands one back, and nothing is logged or thrown.

Smaller change than what I had, too. coordinates-of.js and transformer.js are now main plus a line each instead of BOM arithmetic threaded through every slice. The trimStart thing sorted itself out as well: String#trimStart counts U+FEFF as whitespace, so the old version had to explicitly exclude the BOM to stop columnOffset claiming a column of indent on a file with none. Now trimStart never sees one.

One wrinkle

content-tag strips exactly one BOM. It inherits that from swc, an if and a hardcoded three bytes, and there's no BOM handling in content-tag's own source at all. So src/bom.js has one helper per side of that parse, and they strip different amounts so that they compose:

• stripBomsBeforeParse takes every leading BOM. It's for source on its way into a parser, so swc has nothing left to drop and its offsets land in exactly the string we kept. Used by the Transformer constructor and unprocess.
• stripBomAfterParse takes exactly one, matching swc, to move an already-parsed caller's source into the frame those offsets came from. Used by coordinatesOf and ParseResultStringUtils, both public and both able to receive a caller's buffer directly.

Taking all of them up front is what leaves the single strip on the other side a no-op, and that's what lets ParseResultStringUtils strip defensively for standalone callers without double-stripping when a Transformer built it. Make both take one and they stack instead. Two BOMs through a strip-one-everywhere version mangles the document rather than failing quietly:

export const Foo =<template>Goodbye</template>>

Eaten space, stray >. I didn't hit that on a real file, it fell out of reviewing this branch, but two BOMs reproduce it.

Taking all of them has a cost: a second U+FEFF is really a ZWNBSP, so a document that legitimately starts with one loses it. I think that's the right trade at this size, but I'd rather say so than bury it.

What callers see

start and end index the stripped source, so if you're holding a BOM'd string you can't slice it with those directly. New README section covers it, and the Coordinates type says so too. line, column and columnOffset come out identical for a file with a leading BOM and the same file without one.

I left stripBomsBeforeParse internal instead of exporting it, since your whole point was that users shouldn't have to think about BOMs. Easy to export if you'd rather consumers normalize with the same helper.

Tests

20 in tests/bom.test.ts, across all six public entry points, 0 through 3 BOMs, class member and multi template documents, and Buffer input.

Written test first, and I tried to be honest about which ones actually prove anything:

• The first 13 ran against a no-BOM-handling baseline. 12 failed.
• The three multi-BOM ones failed against the strip-one-everywhere version.
• Disabling each of the three strips on its own fails a distinct set: coordinatesOf's takes out the four coordinate tests, the Transformer constructor's the five output tests, ParseResultStringUtils' just its own.
• The unprocess test and the class member / multi template ones passed on arrival, so they're coverage, not proof. unprocess passed because ember-estree also drops a BOM on reprint. It strips explicitly now so the behavior is ours either way.

pnpm test is 90 green. pnpm lint passes lint:types, lint:package and lint:published-types.

Heads up that CI has never run here. The CI workflow sits at action_required with zero jobs on both commits, waiting on a maintainer to approve workflows for a fork PR, so an empty checks tab isn't telling you anything about the code.

[NullVoxPopuli]

instead of supporting BOM, let's just also strip it, and return the whole content without the BOM

I'm not sure what software is adding a BOM for you, but it is unneeded

[tylerturdenpants]

instead of supporting BOM, let's just also strip it, and return the whole content without the BOM

I'm not sure what software is adding a BOM for you, but it is unneeded

You got it, boss. ❤️

[tylerturdenpants]

Should we fail loudly with a “content tag utils does not support content with BOM” or maybe warn the user that it will be stripped?

[NullVoxPopuli]

why does the user need to know / care they have a BOM or not? we can document that we strip the BOM, but we should not log anything

[tylerturdenpants]

Reworked it to strip instead of offset. Came out smaller than what I had, so thanks for the nudge. Description's rewritten too.

One thing I'd like your call on though.

content-tag strips exactly one BOM, which I think is right: a BOM is only ever the first character, and a second U+FEFF is just a ZWNBSP, so it's content. But that means Transformer can't strip only one. If it does, #originalSource keeps a U+FEFF that content-tag already dropped, and toString() ends up slicing one string with indices from another:

transformSync("" + source, (c) => c.replace("Hello", "Goodbye"));
// "export const Foo =<template>Goodbye</template>>"
//                              ^ eaten space        stray > ^

So the constructor eats every leading U+FEFF, which means a file that legitimately starts with a ZWNBSP after its BOM loses it. No corruption, but it's a character someone typed.

Options as I see them:

1. What's here now. Take them all, document it. A .gjs starting with two U+FEFF is pretty much hypothetical.
2. Stash everything past the first, re-prepend in toString(). No data loss, but that's prefix state on Transformer for a case that'll probably never happen, and it's halfway back to carrying the BOM around, which is what you told me not to do.
3. Throw. Ruled out by "we should not log anything", and throwing's louder than logging.

Went with 1 and documented it in the README. 2 is maybe fifteen lines plus tests if you'd rather keep the character. Genuinely don't mind either way.

Separately, CI has never actually run on this PR. Both commits show CI at action_required with zero jobs, so it's waiting on a maintainer to approve workflows for a fork PR. The empty checks tab isn't telling you anything about the code. Locally it's 90 tests green, and lint:types / lint:package / lint:published-types all pass.

[NullVoxPopuli]

each parse result can have a BOM? how?

[tylerturdenpants]

They cannot, and my comment sent you down the wrong path. Sorry.

ParseResultStringUtils holds one buffer, the whole document. Parse results are just byte ranges pointing into it, they carry no text of their own. So the BOM is a property of that one buffer, once, not of anything per parse result.

Reworded:

// This holds the whole document, which parse results only point into, and
// it is public, so the buffer may be a caller's rather than a Transformer's.

The reason it strips at all is the second half: the class is exported, so someone can do new ParseResultStringUtils(Buffer.from(src)) themselves without a Transformer involved, and then nothing upstream has stripped for them. Constructed by a Transformer the strip is a no-op, since the constructor already handled it.