Skip to content

Conversation

@rogeriochaves
Copy link

@rogeriochaves rogeriochaves commented Feb 14, 2017

So, first of all, I really needed this project going, so I decided to help, then I noted that this is a lot to maintain, congratulations for the awesome work!

What I did was changing the example and use it to test if the stuff worked. Auth was not working, so I needed to upgrade to Firebase 3.0.

But I've had some problems upgrading the javascript, because firebase 3.0 doesn't work when bundled inside something that uses "use strict", as elm does, check it out: https://groups.google.com/forum/#!topic/firebase-talk/gFtK1YoRUoE

So I've decided to delete firebase and rely on the global available one, and found out that this might be a much better approach, because:

  • Firebase is HUGE, it weights 300kb minified, so it is indeed better to serve it separately, from a CDN, or at least to have a separate chunk for it if you are using webpack or something, so it can load in parallel
  • I can upgrade firebase for non-breaking changes without changing elmfire

Going forward, I've realized that the API must change, we have to pass more data than the database url (apiKey, authDomain, databaseURL, storageBucket, messagingSenderId now), and we have to instantiate it without child paths, we could have something like:

firebase : FirebaseInstance
firebase =
    initializeApp
        { apiKey = "foo"
        , authDomain = "bar.firebaseapp.com"
        , databaseURL = "https:/bar.firebaseio.com"
        , storageBucket = "bar.appspot.com"
        , messagingSenderId = "1000000000"
        }

Then, I don't see a reason why the elm api shouldn't just mimic the js api, we could use it like this:

update : Msg -> Model -> ( Model, Cmd Msg )
update msg model =
    case msg of
        Send text ->
            let
                task =
                    firebase.database.ref "mychildpath"
                        |> .set (JE.string text)
            in
                ( model
                , Task.perform
                    (Sent << Err)
                    (Sent << Ok << (always ()))
                    task
                )
        Login ->
            let
                task =
                    firebase.auth.signInWithEmailAndPassword "[email protected]" "123test"
            in
                ( model
                , Task.perform
                    (Sent << Err)
                    (Sent << Ok << (always ()))
                    task
                )

ref = firebase.database();
break;
case 'ref':
ref = getRefStep (rest) .ref (head._1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the spaces after function names?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, that's the way thomas was doing ¯\_(ツ)_/¯

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my style for JS code.
I would appreciate if contributors would stick the style of the existing code.

@ThomasWeiser
Copy link
Owner

@rogeriochaves thank you very much for your contribution. Will review it ASAP.

@rogeriochaves
Copy link
Author

@ThomasWeiser it's not really a valuable code contribution, I opened this PR more for the discussion of what path will we follow

@rogeriochaves rogeriochaves changed the title Effect manager Attempt to use firebase 3.0 Feb 15, 2017
ThomasWeiser added a commit that referenced this pull request Mar 5, 2017
As suggested by Rogério Chaves:
#15 (comment)

Pros:
- No need to patch Firebase script
- Faster loading from CDN / cache / parallel chunk.
- Can upgrade firebase for non-breaking changes without changing ElmFire.
- More flexibility while developing new ElmFire version

Cons:
- ElmFire is not self-contained any more.
- User is now responsibly for compatibility of the version  of included firebase.js script.

This is a preliminary change during development.
We may change back to included Firebase script for published versions of ElmFire.
Needs to be discussed with community.
@ThomasWeiser
Copy link
Owner

As per your suggestion, firebase.js script is now to be included separately. For pros and cons see the commit msg.

Please direct API discussions to #16 or separate issues/PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants