lookup for Headers is not intuitive

[joneshf]

It would seem there's an issue with the casing for Headers. The current behavior is:

HTTPure.lookup (HTTPure.headers "Foo" "bar") "Foo" == Nothing

But, it seems like the behavior should be:

HTTPure.lookup (HTTPure.headers "Foo" "bar") "Foo" == Just "bar"

Seems like the issue is the Lookup instance. As discussed in Slack, probably easiest to use Data.String.CaseInsensitiveString (at least internally).

[joneshf]

Looks like Headers is a newtype around Object String, so using CaseInsensitiveString is out.

I guess there are some options:

1. "Fix" the bug with the current implementation.
   Looks like we'd want to lowercase both header and headers so values going in would be lowercase. It wouldn't really fix the underlying issue as the constructor is exposed and there's a Newtype instance; people can still construct broken Headers.
2. Fix the bug, for real.
   If we do the above, hide the constructor, and drop the Newtype instance, then using Headers from outside the module is consistent.
3. Use Map CaseInsensitiveString String instead of Object String.
   This would fix the issue altogether. It wouldn't require hiding the constructor or dropping the Newtype instance. Dunno if the Object String is a requirement of Headers though.

Thoughts on those three options? Maybe there's another thing to do instead?

[cprussin]

I lean towards option #3, see #113 as I've wanted to remove the use of Foreign.Object for some time and never got around to it.

I think the only reason Foreign.Object is being used is in some olden times, these objects were passed in through the Node APIs more directly than they are now. But now, the Headers.write function just traverses the headers data so there isn't a compelling reason to stick to using Foreign.Object.

[davezuch]

@cprussin any chance of a new release with this fix soon?

[cprussin]

@davezuch so sorry, I meant to release after this merged and then went on vacation and only just returned.

Published v0.8.2

[joneshf]

Thanks for the release!

I think the changes made were breaking though (at least, it makes some of my code not type anymore). Any chance we could get a 0.9.0 release? Maybe also delete the 0.8.2 tag? I dunno how you feel about removing tags that already exist.

[cprussin]

@joneshf I'm surprised that anything in here is breaking, are you relying on any internals in your code? Can you be more specific about what's breaking?

Regardless, I'm actually not really worried about the versioning scheme, because semver specifies that any releases below 1.0.0 may be (though I have tried to stick to minor versions for breaking changes and patch versions for non-breaking).

That said, even though we haven't already finished all the items on my 1.0 wishlist, it might be time to start thinking about cutting a 1.0 release. That will force our hand at being a bit more careful with backwards compatibility.

[joneshf]

Sorry, I thought I responded to this. Will do so for posterity, even though multiple versions have been released since 0.8.2.

---

HTTPure.Headers.Headers was a newtype over Foreign.Object.Object String, then it became a newtype over Data.Map.Map Data.String.CaseInsensitive.CaseInsensitiveString String. That's the breaking change. Code that was working against the Foreign.Object.Object String doesn't work against the Data.Map.Map Data.String.CaseInsensitive.CaseInsensitiveString String.

I'm not sure what's classified as an internal in purescript-httpure, but it doesn't seem like HTTPure.Headers.Headers is internal. The module naming doesn't indicate it should be internal, there doesn't appear to be documentation about it being internal, and it has a Data.Newtype.Newtype instance which makes its underlying representation visible to everyone no matter if it is supposed to be internal. If it's supposed to be internal, maybe documenting it as such, changing the module name, unexposing the constructor, or removing the Data.Newtype.Newtype instance would be useful to indicate that.

If there were some way to work with the concept of HTTPure.Headers.Headers without resorting to unwrapping the newtype, that could work as well. You can put headers into an HTTPure.Headers.Headers without worrying about what the underlying implementation is: HTTPure.Headers.header and HTTPure.Headers.headers. But near as I can tell, there's no way to get back out all of the headers without unwrapping the newtype. The HTTPure.Lookup module only provides a way to get specific headers, not every header.

---

Hope that explains what was happening. Lemme know if something is unclear.

[cprussin]

Got it, thanks for posting. Yes, the intention is to work with headers without unwrapping, but as you're pointing out there are certainly things you can't currently do without unwrapping. Regardless, it's fair to say that even if those existed, changing an exported type is breaking.

In any case, apologies for the semver abuse and thank you for pointing this out!

[joneshf]

No worries. It wasn't that big of an issue, but forgot to follow-up on it.

Would you want to do something about working without unwrapping? We can move to a separate issue if that's better.

[cprussin]

@joneshf honestly I hadn't put too much thought into it, I just had this general feeling that you shouldn't have to unwrap HTTPure newtypes in application code. I'd say if it feels important to anyone, let's go ahead and move to another ticket.