Represents ModuleNames as a single Text#3843
Conversation
The hierarchy `[ProperName]` suggests doesn't actually exist except for the `Prim.` namespace. The compiler compares module names for equality and ordering all over the place though, so we should pick a representation suitable for that.
| $(deriveJSON (defaultOptions { sumEncoding = ObjectWithSingleField }) ''Ident) | ||
| $(deriveJSON (defaultOptions { sumEncoding = ObjectWithSingleField }) ''ModuleName) | ||
|
|
||
| instance ToJSON ModuleName where |
There was a problem hiding this comment.
I'm continuing to encode module names as arrays of strings in JSON to not break the compatibility tests. I'm assuming changing this would break Pursuit. Would Pursuit be fine if my handwritten instance accepted both a single string as well as an array of them?
|
I think we just need to be aware of how changes like this will affect CoreFn, which is a stable API. |
|
Pursuit would probably be fine if we changed the format; we've made changes to JSON instances in a backwards-compatible way for Pursuit before, i.e. changing the format we generate but keeping old versions of parsers around and attempting the older parser if the new parser fails to parse. The only issue I'm aware of with that is that we need to update Pursuit quickly after a new release when we do that because older compilers won't be able to parse the new JSON of course. That should be easier now that more people can deploy to the Pursuit server, though. However, I think changing the JSON format is more of an issue for corefn, as @natefaubion points out. Do you think we would gain a lot from changing the JSON encoding to use a single string? At the moment this doesn't seem worth breaking people's corefn parsers to me; I think I would prefer to retain all of the same JSON instances (i.e. convert to/from a list of strings during encoding/decoding) and keep this change strictly internal. |
|
Thanks for pointing out that CoreFn shouldn't change for something like this. I've changed the en/decoders to match our previous representation. |
|
Thanks for the review! Let's hope I don't cause too many conflicts in open PRs... |
* Represents ModuleNames as a single Text The hierarchy `[ProperName]` suggests doesn't actually exist except for the `Prim.` namespace. The compiler compares module names for equality and ordering all over the place though, so we should pick a representation suitable for that. * preserve CoreFn encoding
The hierarchy
newtype ModuleName = ModuleName [ProperName]suggests doesn't actually exist except for thePrim.namespace.The compiler compares module names for equality and ordering all over the place though, so we should pick a representation suitable for that. This improves my extern loading benchmarks by another 10%.
Most of that will be the more compact encoding, that I could get with a handwritten instance as well, but I wanted to suggest this representation for a while now. What do you think?