purescript / purescript-strings

String utility functions, Char type, regular expressions.
BSD 3-Clause "New" or "Revised" License
54 stars 71 forks source link

Should `localeCompare` return in `Effect`? #169

Open pete-murphy opened 9 months ago

pete-murphy commented 9 months ago

localeCompare can return different output depending on locale. Example borrowed from MDN:

console.log("ä".localeCompare("z", "de")); // a negative value: in German, ä sorts before z
console.log("ä".localeCompare("z", "sv")); // a positive value: in Swedish, ä sorts after z

The FFI for Data.String.Common.localeCompare uses the system default locale, so Data.String.Common.localeCompare "ä" "z" could be LT or GT depending on if it's run in Germany or Sweden, for example.

It seems unlikely (maybe even impossible? I'm not sure) for a system's locale to change over the lifetime of a program, so maybe that is enough to consider localeCompare observationally pure?

I'm mostly just curious about the intuition for what makes FFI code "effect"-ful, as I have a library of bindings for the Intl object and I was planning on adding to it a localeCompare that accepted the Locale argument as well.

garyb commented 9 months ago

I agree, this does seem like it should be in Effect since it's based on the environment it's being run in.

michaelficarra commented 9 months ago

I confirmed with the Intl authors that a JS program may observe the system locale change over the course of its execution, so localeCompare should indeed be considered impure.

pete-murphy commented 9 months ago

I could make a PR to update the type here (along with the docs and tests), but looking at the rest of the library, this would be the first use of Effect outside of test code. I'm wondering if there's another discussion to be had around whether this function belongs in the library?

Tracking down when localeCompare was added, I was surprised to find it's been here since the beginning—a decade ago!

michaelficarra commented 9 months ago

Well it's still useful when passed a locale explicitly. Maybe just require the locale parameter? Or is there maybe a more appropriate module for locale-sensitive operations to live in?

pete-murphy commented 9 months ago

Or is there maybe a more appropriate module for locale-sensitive operations to live in?

I've been working on bindings for the locale-sensitive operations specified in ECMA 402 here: https://github.com/pete-murphy/purescript-js-intl/tree/main/src/JS/LocaleSensitive. From what I remember of last time I looked, I couldn't find any other references to any of those functions on Pursuit aside from this library's localeCompare.

Maybe just require the locale parameter?

What's the type of the locale parameter? If it's String then it can throw

"a".localeCompare("b", "zzzz") // throws a RangeError

Even if it were a well-typed Locale or Array Locale you'd have to account for the runtime not supporting localeCompare for that locale and falling back to the system locale.

michaelficarra commented 9 months ago

What's the type of the locale parameter? If it's String then it can throw

That's a good point. Then I support dropping it or moving it to another more appropriate place.

garyb commented 9 months ago

I think it seems reasonable to drop it here :+1: