Enabled firebase to be called as function, thus enabling patterns where Firebase ref's are stored in root component #25

DaBs · 2016-08-10T21:03:19Z

This pull request will enable using the firebase option in the same way as the data option, by calling it as a function and returning an object. This allows for coding patterns where you originally define the Firebase references as data in the root component, but want to use the later on in child components, e.g.

export default {
    ...,
    firebase() {
        return {
            someArray: this.$root.database.ref('someArray')
        }
    },
    ...
}

jwngr · 2016-08-10T21:07:25Z

Can you just use firebase.database.ref('someArray') instead this.$root.database.ref('someArray')? That should make it so that you don't need to have a function here and it's even fewer characters.

DaBs · 2016-08-10T21:27:57Z

Hm, sure, albeit this means you have to (with ES6 modules atleast) import the firebase module everywhere you want to use it, which is then a lot more characters than the above mentioned (if that's what we're measuring). Furthermore, I don't understand why this would not be intended behaviour, seeing as some other VueJS options. like data, is capable of being defined this way, and that the VueFire basicly mirrors the data options to a degree.

jwngr · 2016-08-10T21:30:59Z

The fewer characters was a bit tongue-in-cheek. I'll leave it to Evan to comment on whether or not this makes sense from a Vue perspective. From a Firebase perspective though, I think it would be a bit redundant.

DaBs · 2016-08-10T21:34:43Z

Heh, well aware, I was just throwing it right back at you in the same manner. 😛 No offence taken. And I realise that this is sort of opposite of the idea behind how Firebase wants to both store its own state and references, but attempting to utilize this in a VueJS component context, it made little sense why it wouldn't behave like the data() { ... } option, as it to some degree replaces that in a VueJS + Firebase app.

jwngr · 2016-08-10T21:40:11Z

Haha makes sense 😄 I am not familiar enough with VueJS to say whether or not this is a good idea. It certainly seems like it takes very little code to support and I guess I don't see a reason not to include it, but I'll leave that to Evan to decide.

Thanks for the contribution by the way! I'm always happy to see the community getting involved!

DaBs · 2016-08-10T22:01:24Z

No problem, I'm just happy that two of my favourite stack choices right now have a well-functioning module and I'll gladly contribute to ensure that it continues to be well-functioning 😄 Now we just wait for Evan

posva · 2016-08-11T07:47:09Z

@DaBs Can you add a test, please?
Also, I think you can remove the compiled version. That seems to be done in a separate commit

DaBs · 2016-08-11T14:02:04Z

@posva I'm not sure if this covers what you wanted from a test case, but it tests the basic functionality of whether or not you can use the firebase option as a function. And the compiled version seems to get created as a build artifact when you run the tests?

posva · 2016-08-11T14:40:15Z

tests/vuefire.spec.js


  beforeEach(function (done) {
-    firebaseRef = firebaseApp.database().ref()
+    firebaseDb = firebaseApp.database()


This can be moved inside the it function as it's only used there

posva · 2016-08-11T14:45:02Z

@DaBs Yes, thank you.
About the compiled version, you'll have to rebase and remove the modification in the build. You have to commit everything but the dist files.

posva · 2016-08-11T14:49:25Z

tests/vuefire.spec.js

@@ -25,6 +27,36 @@ describe('VueFire', function () {
    })
  })

+  describe('is callable as Function', function () {


The title is missing something about option, which is what you're actually changing. I'd name it something like support Function options

This reverts commit 327203a.

This reverts commit a11bedd.

This reverts commit 327203a.

posva · 2016-08-11T19:51:57Z

tests/vuefire.spec.js

+        components: {
+          'child-component': Vue.extend({
+            name: 'ChildComponent',
+            firebase: function () {


Do you actually need to have a child component?

It could easily be simplified to simply test the call of the function. I'll change immediately.

DaBs · 2016-08-15T09:54:30Z

@posva Does the test satisfy the standard? I'm sorry it took so long to get a proper test, but writing unit tests in chai is relatively new for me 😄

posva · 2016-08-15T22:38:23Z

@DaBs It's ok, I actually feel being pushy 😓
The unit test is much better that way. It'd be perfect if you can use a spy and check it gets called only once after the $mount
I'd also remove the regex on the throw. If it throws anything it can be considered as a failure

DaBs · 2016-08-23T06:22:41Z

@posva Sorry for the delay, I was busy most of the week. The latest commit should have added both a spy and removed the regex 😄

posva · 2016-08-23T07:48:03Z

src/vuefire.js

@@ -198,6 +198,7 @@ function ensureRefs (vm) {
 var init = function () {
  var bindings = this.$options.firebase
  if (!bindings) return


I think it makes sense to move this one line below, so if the function is returning nothing we can skip too

posva · 2016-08-23T07:49:43Z

@DaBs That's okay 😄

posva · 2016-08-23T16:37:46Z

src/vuefire.js

@@ -196,8 +196,9 @@ function ensureRefs (vm) {
 }

 var init = function () {
+  if (!this.$options.firebase) return


I wasn't clear about this, sorry. I was thinking about this:

var bindings = this.$options.firebase if (typeof bindings === 'function') bindings = bindings.call(this) if (!bindings) return

DaBs · 2016-08-30T08:36:51Z

What is status on this PR? 😄

posva · 2016-08-30T08:41:07Z

@DaBs Sorry for not giving you any updates! Can you remove the modifications on the dist folder, so we only keep the src and test modifications?

DaBs · 2016-08-30T10:03:32Z

@posva Ah, damn, that reoccured. Will do ASAP.

DaBs · 2016-08-31T20:05:58Z

@posva This should do it 😄

posva · 2016-09-09T16:35:13Z

@DaBs Thank you very much! I'm merging this and adding some minor changes to the test!

Enabled firebase to be called as function

5f52e88

Added basic test case for ability to run as function

852cd5b

Deleted compiled files

327203a

posva reviewed Aug 11, 2016
View reviewed changes

posva and others added 4 commits August 11, 2016 17:55

Revert "Deleted compiled files"

a11bedd

This reverts commit 327203a.

Revert "Revert "Deleted compiled files""

f9c7112

This reverts commit a11bedd.

Revert "Deleted compiled files"

530dfef

This reverts commit 327203a.

Changes to test spec

3e67330

posva reviewed Aug 11, 2016
View reviewed changes

Updated test to be more simple

8af7c23

Removed regex from throw, added Sinon spy to call

9846ea9

posva reviewed Aug 23, 2016
View reviewed changes

Changed line order of check

5c5acc6

posva reviewed Aug 23, 2016
View reviewed changes

Moved the correct line :P

5c39b0e

DaBs added 2 commits August 31, 2016 00:01

Reverted changes to dist files

80c24fc

Merge branch 'master' of github.com:DaBs/vuefire

46e7003

posva mentioned this pull request Sep 8, 2016

Querying with params from $route #10

Closed

posva merged commit 195ea7d into vuejs:master Sep 9, 2016

DaBs mentioned this pull request Oct 26, 2016

Can't access $route params in .vue component #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled firebase to be called as function, thus enabling patterns where Firebase ref's are stored in root component #25

Enabled firebase to be called as function, thus enabling patterns where Firebase ref's are stored in root component #25

DaBs commented Aug 10, 2016 •

edited

Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 •

edited

Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 •

edited

Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 •

edited

Loading

posva commented Aug 11, 2016

DaBs commented Aug 11, 2016 •

edited

Loading

posva Aug 11, 2016

posva commented Aug 11, 2016

posva Aug 11, 2016

posva Aug 11, 2016

DaBs Aug 11, 2016

DaBs commented Aug 15, 2016

posva commented Aug 15, 2016

DaBs commented Aug 23, 2016

posva Aug 23, 2016

posva commented Aug 23, 2016

posva Aug 23, 2016

DaBs commented Aug 30, 2016

posva commented Aug 30, 2016

DaBs commented Aug 30, 2016

DaBs commented Aug 31, 2016

posva commented Sep 9, 2016

Enabled firebase to be called as function, thus enabling patterns where Firebase ref's are stored in root component #25

Enabled firebase to be called as function, thus enabling patterns where Firebase ref's are stored in root component #25

Conversation

DaBs commented Aug 10, 2016 • edited Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 • edited Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 • edited Loading

jwngr commented Aug 10, 2016

DaBs commented Aug 10, 2016 • edited Loading

posva commented Aug 11, 2016

DaBs commented Aug 11, 2016 • edited Loading

posva Aug 11, 2016

Choose a reason for hiding this comment

posva commented Aug 11, 2016

posva Aug 11, 2016

Choose a reason for hiding this comment

posva Aug 11, 2016

Choose a reason for hiding this comment

DaBs Aug 11, 2016

Choose a reason for hiding this comment

DaBs commented Aug 15, 2016

posva commented Aug 15, 2016

DaBs commented Aug 23, 2016

posva Aug 23, 2016

Choose a reason for hiding this comment

posva commented Aug 23, 2016

posva Aug 23, 2016

Choose a reason for hiding this comment

DaBs commented Aug 30, 2016

posva commented Aug 30, 2016

DaBs commented Aug 30, 2016

DaBs commented Aug 31, 2016

posva commented Sep 9, 2016

DaBs commented Aug 10, 2016 •

edited

Loading

DaBs commented Aug 10, 2016 •

edited

Loading

DaBs commented Aug 10, 2016 •

edited

Loading

DaBs commented Aug 10, 2016 •

edited

Loading

DaBs commented Aug 11, 2016 •

edited

Loading