!bigint tag

[isaacs]

I think it'd be nice if there was a custom tag to explicitly tag BigInt values.

However, this isn't quite working. It seems like yaml.stringify() is turning BigInts into Numbers, even if the tag identifies the value.

Haven't dug into it more than a cursory glance to see that intAsBigInt behavior seems to perhaps be conflicting with what this is trying to do, and it's a nice-to-have at best, so if it's too hard to figure out, then meh. If there's some obvious thing I'm missing, then I'll try to get this over the finish line. :)

[eemeli]

That sounds like it's because the !!int tags are grabbing the BigInt values before !bigint. The relevant bit of code is here.

So to get something like !bigint to work, there are two current solutions:

1. Use the schema: 'core' option, as that only includes !!str for scalars.
2. Use a function for customTags that prepends rather than appends your custom tags.

We should probably include an example of the latter in the README here.

[isaacs]

2. Use a function for customTags that prepends rather than appends your custom tags.

This might be a better question to post on yaml rather than here, but is there any case where you provide custom tags, and don't want them to take priority over the built in tags? It seems like if I explicitly pass in customTags, that's what should take priority, so the result should be tags = customTags.concat(tags) (or if the issue is that customTags can contain arrays, maybe flatten with tags = customTags.concat(tags).reduce((s: Tags, tag) => s.concat(tag), [])).

[isaacs]

Updated to prepend the tag to the list, and now it does work as expected :)

[eemeli]

Other than the regexp fix, this looks good!

In retrospect, I think you're right and custom tags should've been prepended rather than appended by default. Given that there is a workaround, I'd really rather not risk breaking things by changing that anymore. :/

[eemeli]

This is missing negative numbers and 0. Tests should also cover those.

Suggested change

	const match = str.match(/^([1-9][0-9]*|0x[0-9a-fA-F]+|0o[0-7]+|0b[01]+)n?$/)
	const match = str.match(/^(0|-?[1-9][0-9]*|0x[0-9a-fA-F]+|0o[0-7]+|0b[01]+)n?$/)

[eemeli]

The suggestion is a subset of that supported by JS, as BigInt('0000001') and BigInt('-0') are also valid. Not aware of a real reason why they should be supported though.

[isaacs]

I think it's fine for the YAML expression of BigInt to use canonical integer forms. Really, even having hex/octal/binary is just a flex 😅

[eemeli]

Personally I'd inline this to reduce unnecessary abstraction, but that's just a preference.

[isaacs]

The indirection saves a bit of extra annotation that typescript requires when using a method in the object initializer (because identify() is called in the stringify method). But I don't have a strong preference there.

[isaacs]

I guess we already had satisfies ScalarTag so it doesn't save much anyway, can just export bigint: ScalarTag = ...

[isaacs]

Simplest solution was just to support the full range of 0000123n that JS allows. 💪

PTAL, thanks :)

[isaacs]

Given that there is a workaround, I'd really rather not risk breaking things by changing that anymore. :/

I think using custom tags at all is probably a pretty advanced feature, so I agree the right approach probably is to just leave it as-is and maybe just add a caveat in the docs.

[eemeli]

I think this matches just - and an empty string, which isn't intentional.

I'm also not convinced about supporting negatives for non-decimals, as e.g. BigInt('-0x1') throws an error. While YAML numbers support negative non-decimal values, this encoding is specifically for JS BigInt, for which there's an exact spec for valid input. We should not extend support for parsing other strings outside that.

[isaacs]

Ah, I didn't realize you couldn't do negative hex/oct/bin numbers, since that works for non-bigint.

Seems like this matches it?

diff --git a/src/bigint.ts b/src/bigint.ts
index 8444f72..955281a 100644
--- a/src/bigint.ts
+++ b/src/bigint.ts
@@ -14,11 +14,10 @@ export const bigint: ScalarTag = {
   tag: '!bigint',
   resolve(str: string) {
     const match = str.match(
-      /^(-?)([0-9]*|0x[0-9a-fA-F]+|0o[0-7]+|0b[01]+)n?$/
+      /^(-?[0-9]+|0x[0-9a-fA-F]+|0o[0-7]+|0b[01]+)n?$/
     )
     if (!match) throw new Error('Invalid BigInt value')
-    const bi = BigInt(match[2])
-    return match[1] ? bi * -1n : bi
+    return BigInt(match[1])
   },
   stringify(item: Scalar, ctx: StringifyContext, onComment, onChompKeep) {
     if (!bigint.identify?.(item.value)) {

Or I suppose it could just accept anything and throw if the cast fails, which would avoid the need to even have a regexp?

diff --git a/src/bigint.ts b/src/bigint.ts
index 8444f72..ef216c8 100644
--- a/src/bigint.ts
+++ b/src/bigint.ts
@@ -13,12 +13,12 @@ export const bigint: ScalarTag = {
   },
   tag: '!bigint',
   resolve(str: string) {
-    const match = str.match(
-      /^(-?)([0-9]*|0x[0-9a-fA-F]+|0o[0-7]+|0b[01]+)n?$/
-    )
-    if (!match) throw new Error('Invalid BigInt value')
-    const bi = BigInt(match[2])
-    return match[1] ? bi * -1n : bi
+    const match = str.match(/^(.*?)n?$/)
+    try {
+      return BigInt(match?.[1] || 'invalid')
+    } catch {
+      throw new Error('Invalid BigInt value')
+    }
   },
   stringify(item: Scalar, ctx: StringifyContext, onComment, onChompKeep) {
     if (!bigint.identify?.(item.value)) {

[isaacs]

Oh, you can do negative bin/oct/hex bigints as literals, just not with the string cast. Ie:

> -0xDEADBEEFn
-3735928559n
> BigInt('-0xDEADBEEF')
Uncaught SyntaxError: Cannot convert -0xDEADBEEF to a BigInt

So I guess the question is whether the restriction on the yaml expression should be "code that is a bigint literal" or "a string that can be passed to the BigInt function".

If it's the first, then -0xf should be allowed. Since it includes the trailing n by default in the stringified form, it seemed like that would be the expectation.

[isaacs]

Oh, but, yaml.parse('-0xf') parses as a string. So yeah, probably better to not support it for BigInt if it's not supported for Number.

[eemeli]

Or I suppose it could just accept anything and throw if the cast fails, which would avoid the need to even have a regexp?

I like that idea. Could even drop the try-catch, as the errors thrown by BigInt() look pretty decent.

I'm a bit surprised that BigInt('') parses to 0n, happy to error on that too.

[isaacs]

Replaced the regexp with just a test to remove the n from the end of the string if present. (Could also remove that if the stringify() didn't put it there, but maybe it shouldn't?)

This does make some of the tests kind of redundant, since now they're just testing the JS BigInt() implementation, but seems harmless to leave in 😅

[eemeli]

I like this. Let's keep the trailing optional n.

[isaacs]

Let's keep the trailing optional n.

I like it too. Makes it feel more biginty.