ChristophP / elm-i18next

https://package.elm-lang.org/packages/ChristophP/elm-i18next/latest
BSD 3-Clause "New" or "Revised" License
67 stars 13 forks source link

Question: Dealing with null #24

Closed os6sense closed 4 years ago

os6sense commented 4 years ago

I have a service which will sometimes return null rather than a string value which causes I18next to fail. In an attempt to work around this I have a re-implementation of the treeDecoder to deal with this case.

I'm running into a problem in my code though where the variants of Tree (Branch/Leaf) are not exposed.

I was wondering if the decision not to expose the variants was deliberate, or you would consider accepting a PR exposing them; or alternatively, would accept a PR to handle null values in the JSON?

ChristophP commented 4 years ago

Hi @os6sense , hmm seems like null values are causing a bit of a headache here.

To your question: Yes, the choice to keep the constructors hidden was deliberate, to allow me to make changes to internals without breaking users code. However, due to an older issue I have exposed fromTree, string and object functions which basically wrap those constructors. https://package.elm-lang.org/packages/ChristophP/elm-i18next/latest/I18Next#fromTree Would those help you?

Regarding allowing null. The question I am asking myself is: Doesn't null indicate some sort of error or absence of a translation that should be there in your case? What you happen i you use that missing value in your application?

os6sense commented 4 years ago

Hi @ChristophP, yes the null is an annoyance. For my purposes I can't (consistently) address the cause of the null so I need to ensure the encoderdecoder is robust enough to deal with it e.g. substituting an empty string.

I'm new to Elm so struggling with this one; I'm aware of the constructors but without access to the variants I can't e.g. write a case statement to handle the flattening as in flattenTranslationsHelp https://github.com/ChristophP/elm-i18next/blob/master/src/I18Next.elm#L168.

Alternatively because flattenTranslations isn't exposed I can't use that to deal with the result of a custom treeDecoder.

A similar issue exists with the Translations variant as well ... with a custom decoder I can't use the Translations variant and get compilation errors. I just tested and with Translations(..) and flattenTranslations exposed though I'm able to write a custom Translations/Tree decoder handling nulls.

As said though, I'm new to elm and there may be an approach I've missed ... a colleagues solution was to copy and patch the code :scream: which I'd rather avoid.

os6sense commented 4 years ago

Something along the lines of the following is what I'm attempting BTW:

module I18Extra exposing (translationsDecoder)

import Dict exposing (Dict)
import I18Next exposing (Translations(..), Tree)
import Json.Decode as Decode exposing (Decoder)

translationsDecoder : List String -> Decoder ( List String, Translations )
translationsDecoder unsafeKeys =
    Decode.dict treeDecoder
        |> Decode.map (I18Next.flattenTranslations >> collectMissingValues unsafeKeys)

collectMissingValues : List String -> Dict String String -> ( List String, Translations )
collectMissingValues unsafeKeys dict =
    ( List.concat
        [ Dict.values (Dict.filter (==) dict)
        , List.filter (\key -> not (Dict.member key dict)) unsafeKeys
        ]
    , Translations dict
    )

treeDecoder : Decoder Tree
treeDecoder =
    Decode.oneOf
        [ Decode.string
            |> Decode.map
                (\val ->
                    if String.isEmpty (String.trim val) then
                        I18Next.string ""

                    else
                        I18Next.string val
                )
        , Decode.null (I18Next.string "")
        , Decode.lazy
            (\_ -> Decode.dict treeDecoder |> Decode.map (\v -> Dict.toList v |> I18Next.object))
        ]

For the above to work I exposed Translation(..) and flattenTranslations in I18Next.

ChristophP commented 4 years ago

Hi @os6sense ,

I see you can't pattern match on the Tree values with the current exports. You're right flattenTranslations is not exposed but it is used internally in fromTree. I think you could build your own decoder kind of like this(haven't tested if it compiles):

import I18Next

treeDecoder : Decoder (List (String, Tree))
treeDecoder =
    Decode.keyValuePairs <|
      Decode.oneOf
          [ Decode.string
              |> Decode.map
                  (\val ->
                      if String.isEmpty (String.trim val) then
                          I18Next.string ""

                      else
                          I18Next.string val
                  )
          , Decode.null (I18Next.string "")
          , Decode.lazy (\_ -> treeDecoder)
          ] 

translationsDecoder : Decoder Translations
translationsDecoder =
   Decode.map I18Next.fromTree treeDecoder

Would that work for you?

If not I am willing to consider exposing something like a sloppyDecoder that allows null as well. But before I'd like to research a bit how common that is and if there's a something like a spec for these translation files and what they say about null values.

os6sense commented 4 years ago

Hi @ChristophP, thanks for the suggestion, it doesn't compile off the bat but I'll play around some more.

At the end of the day translations can be just about anything. The root source of the problem I'm looking at here is an endpoint that more or less regurgitates YAML as JSON, and since null is a valid JSON value, it happily spits it back out (along with all other sorts of junk). I won't go into the complexities of why this is so (:grimacing:) but it's not that uncommon.

I think direct support for null values is actually less valuable than having the ability for a user of I18Next to write their own decoders to deal with their use cases (which might be even more extreme) ... perhaps making the decode pipeline more pluggable?

I'll let you know how it goes :+1:

ChristophP commented 4 years ago

@os6sense Right just looked over the code again and realized I didn't use I18Next.object at all. Changing the last case in the oneOf to this should work, does it:

Decode.lazy (\_ -> treeDecoder |> Decode.map I18Next.object)

Don't have a laptop to test right now, sorry.

ChristophP commented 4 years ago

@os6sense So I tried it again and this one compiles: https://ellie-app.com/8wFhxmjswGYa1

os6sense commented 4 years ago

@ChristophP

Thanks for that. I'm sadly having to juggle issues myself at the moment and the solution has been (for now) to modify the source. I'm hoping to be able to pivot at some point soon to look at this again but feel free to close for now.

Again, many thanks for the help :+1:

ChristophP commented 4 years ago

@os6sense Sure. If at one point you're free again. The link I posted should contain pretty much everything you need to address your issue with a lot less code than copying the source. Closing for now. Feel free to open another issue when help is needed.