[colinwahl]

This morning @thomashoneyman and I were working on adding more comprehensive tests for the Registry.Index module, when we noticed that the Manifest type's JSON encoding/decoding doesn't roundtrip into an equivalent value.

We expected it would, so I put this test together to get to the root cause.

I've isolated the cause to our handling of the Range type - specifically, when we encode it, we use the converted entry, which may not be the same as the original entry.

Swapping printRange to print the original entry makes these tests pass, but makes the existing Parse Range "^1.3.4" into ">=1.3.4 <2.0.0-0" fail.

For now I haven't applied any changes to the implementation of printRange, as I wanted to double check with @f-f and @thomashoneyman about what the expected behavior is.

[f-f]

The JSON encoding of Manifest is not supposed to roundtrip - as you noticed, parsing from JSON will convert the SemVer ranges, and we write back the converted value.
This is by design, because we value "make sure that we write Manifests that conform to our validations" (and that includes "the SemVer ranges look in a certain way") more than "the JSON encodings always roundtrip"

However, a proper Manifest (I.e. with the ranges converted) should roundtrip properly. Have you found that this is not the case?

Also, I'm not sure I understand what's the goal of the tests being added in this PR?

[thomashoneyman]

The JSON encoding of Manifest is not supposed to roundtrip - as you noticed, parsing from JSON will convert the SemVer ranges, and we write back the converted value.

This is by design, because we value "make sure that we write Manifests that conform to our validations" (and that includes "the SemVer ranges look in a certain way") more than "the JSON encodings always roundtrip"

I think the real issue is that the Eq instance for Manifest will consider two manifests to be different from one another if their Range types differ because of the original field. For example:

range1 = { original: "^1.0.0", converted: ">=1.0.0 <=2.0.0-0" }
range2 = { original: ">=1.0.0 <=2.0.0-0", converted: ">=1.0.0 <2.0.0-0" }

This means that if you take a Manifest with the first range, then you encode it and decode it from JSON, now you have a Manifest with the second range and the two manifests are not considered to be equal according to Eq. So perhaps the bigger issue here is:

1. Do we need to store the Range object? What is the value of preserving the original field?
2. If we do, then should we implement an Eq instance that only relies on the converted field? After all, two manifests that had different input version strings in a target probably ought to be considered the same because their parsed version range is the same. What benefit is there to considering these manifests "not equal"?
3. If we want to preserve the existing well-behaved Eq instance, then we probably ought to implement a new eqManifest function that allows you to sort of check equality, by ignoring the "raw" input to the manifest. We might also want to remove the Eq instance and implement a separate strictEqual function because it's so easy to make a mistake like this.

However, a proper Manifest (I.e. with the ranges converted) should roundtrip properly. Have you found that this is not the case?

Correct: if the original and converted fields are already the same, then the manifest does round-trip. However, none of the files we import from Bower, for example, are going to round-trip.

Also, I'm not sure I understand what's the goal of the tests being added in this PR?

The goal of these tests were to isolate issues we ran into adding tests for the registry index. We'd write a bunch of manifests out to the registry index, and then read them back, and check the on-disk registry index against the in-memory registry index and find that they were failing an Eq check. We were trying to understand why the manifests would be different when written to disk and then read back and needed to isolate the issue with encoding and decoding.

[f-f]

Do we need to store the Range object? What is the value of preserving the original field?

I have memory of us pairing on this, so I was hoping you'd remember 😄
IIRC we needed it at some point, but there seems to be no trace of useful usage at this point, so maybe we can remove it?

then we probably ought to implement a new eqManifest function that allows you to sort of check equality

Yea, if we have to use a different Eq semantics I'd prefer that we add another function, like compareManifest

The goal of these tests were to isolate issues we ran into adding tests for the registry index

I see now, thanks!

[thomashoneyman]

I have memory of us pairing on this, so I was hoping you'd remember 😄
IIRC we needed it at some point, but there seems to be no trace of useful usage at this point, so maybe we can remove it?

We might have just been parsing into the Range type and then either used that field for errors or just didn't bother going any further 😆 @colinwahl and I can clean up the type a little bit since we don't seem to use that information at all, and then fix these tests and continue on with the registry index tests.

[f-f]

Looks great!