Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add basePath property to firebase environment configuration (for #496) #513

Closed
wants to merge 4 commits into from

Conversation

elgordino
Copy link

@elgordino elgordino commented Jun 4, 2017

Description

This PR adds a new configuration option for the firebase environment properties called basePath. When set this causes all Firebase references to be prefixed by the requested pat.h

This is intended to be used when wanting to 'chroot' an emberfire environment to a specific location in the Firebase database, such as when running multiple environments concurrently in the same database.

Code sample

var ENV = {
  firebase: {
    apiKey: 'xyz',
    authDomain: 'YOUR-FIREBASE-APP.firebaseapp.com',
    databaseURL: 'https://YOUR-FIREBASE-APP.firebaseio.com',
    storageBucket: 'YOUR-FIREBASE-APP.appspot.com',
    basePath: 'your/base/path'
  }

Documentation

I have not updated the documentation as I wasn't sure where to note this option, it seems a bit advanced to display on the main guide installation page.

Tests

I have created some very basic tests, I wasn't sure how to approach more detailed testing as the mocking overrides the ref anyway.

The execution sometimes generates this error, caused in destroy-firebase-apps.js sometimes the execution works fine. I'm not sure what's happening here.

not ok 63 Firefox 53.0 - Integration: FirebaseAdapter - Base Path "after each" hook for "has a Firebase ref"
---
    message: >
        e is undefined
    stack: >
        n@http://localhost:7357/assets/vendor.js:66827:3325
        t/<@http://localhost:7357/assets/vendor.js:66827:3234
        t@http://localhost:7357/assets/vendor.js:66827:3193
        removeApp@http://localhost:7357/assets/vendor.js:66827:4422
        value/<@http://localhost:7357/assets/vendor.js:66827:5999
        
    Log: |
...

@mileszim
Copy link

mileszim commented Jun 5, 2017

I'm not sure I'm completely in favor of this PR, as this feature exists in a different form https://github.com/firebase/emberfire/blob/master/addon/adapters/firebase.js#L45-L60

@elgordino
Copy link
Author

I've discussed the comment in adapter/firebase.js with @mileszim in the #e-emberfire chat. The functionality described in the comment does not appear to work. Attempting this

import Firebase from 'firebase';
export default FirebaseAdapter.extend({
    firebase: new Firebase('https://...')
});

Results in TypeError: _firebase.default is not a constructor

I suspect this behaviour might have been removed when migrating to 2.x.x?

Copying the addon/service/firebase.js to a new service with my suggested modification then injecting it in the adapter extension like this does work:

import FirebaseAdapter from 'emberfire/adapters/firebase';
import Ember from 'ember';

export default FirebaseAdapter.extend({
    firebase: Ember.inject.service('lnfirebase')
});

I could just do this for my project, but I feel the functionality to be able to chroot an emberfire install is worth considering for merging.

@mileszim
Copy link

mileszim commented Jun 6, 2017

I am in favor of this PR now based on the discussions we had, as @elgordino mentioned

@jamesdaniels
Copy link
Contributor

jamesdaniels commented Jan 23, 2018

I'm alright with this while we consider options in the next version, which is on it's way soon. I've fixed the flaky tests, could you look into the remaining failure @elgordino ?

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