Global namespace pollution; `stringRefs` ?

[BurtHarris]

Reading decode.js it looks like there is an undeclared variable stringRefs. Am I correct that the implementation of tags 25 and 256 is incomplete?

Perhaps the following should be commented out to avoid treating stringRefs as a property of the global object.

currentExtensions[25] = (id) => {
	return stringRefs[id]
}
currentExtensions[256] = (read) => {
	stringRefs = []
	try {
		return read()
	} finally {
		stringRefs = null
	}
}
currentExtensions[256].handlesRead = true

I'll submit a PR including that if you agree.

[kriszyp]

Yes, you are correct, that is a global and should have been scoped to the module. A PR is welcome, thank you!

[BurtHarris]

Digging in, I see that stringRefs decode is not complete. I looked into completing it, but interactions with bundled strings would make the code ugly. And test cases exercising the logic would be hard to come by...

So actually my suggestion will be to remove the stringRef extension table entries.

[here-abarany]

And test cases exercising the logic would be hard to come by...

If you want to create examples, Python's cbor2 module supports stringrefs. The main thing to test for is the correlation between the string length and index size when determining whether to add a string to the reference table. (see also the stringref reference, which has some examples as well)