Native review for thomasweiser/elmfire about package.elm-lang.org HOT 6 CLOSED

After giving this a once-over, it looks like it uses Tasks appropriately, and that the native code is necessary (since Firebase.js seems to do a lot of special stuff). Not sure if I have the authority to approve this, but it looks good to me.

from package.elm-lang.org.

The API looks a bit awkward to use. But I believe having these primitives is necessary if we want someone to build a nicer API on top of it.

I approve native use for this library.
We need a third vote to add it to the whitelist: @evancz @rtfeldman

from package.elm-lang.org.

There's a lot going on here, but I can't identify any specific problems. Let's add it!

I approve native use for this library.

from package.elm-lang.org.

Is it possible for any facts about Reference to change dynamically? If so, I think the following functions would have issues:

key : Reference -> String
toUrl : Reference -> String
location : Reference -> Location

I don't understand things well enough to totally understand if Reference is "immutable" or not. Generally speaking, if you want to know something about a value that can mutate, the result needs to be a task.

Also, the discussion about removing record extension and deletion ended with those things getting removed. So the part of the API using { r - x | y = abc } is all going to stop working with 0.16. I don't know if that is a blocker, but it is certainly a big change that'd need to happen here. Reviewers, what do you think of this?

from package.elm-lang.org.

Thanks for all the reviews!

@evancz: Yes, Reference is immutable, so the functions key, toUrl and location are pure.

About the API for query options: Yes, I will change it in the way discussed here to move away from record field addition/deletion and use union types instead. The necessary changes in native code will be limited to the function queryOrderAndFilter, which translates the query options from their Elm representation to the native Firebase representation.

from package.elm-lang.org.

Alright, thanks everyone for reviewing here :)

@ThomasWeiser, you are on the whitelist now, you should be able to put it in the catalog now!

from package.elm-lang.org.