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

Firestore #327

Open
wants to merge 21 commits into
base: firestore
Choose a base branch
from
Open

Firestore #327

wants to merge 21 commits into from

Conversation

Westbrook
Copy link

Fixes #326
Updates #279 as per the mixin format

  • Prevents race conditions on setting up the DB.
  • Prevents multi-splicing on live data updating to collections bound with more than one template variable.

Copy link
Contributor

@merlinnot merlinnot left a comment

Choose a reason for hiding this comment

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

I see the reasoning behind this PR, but I’d like to clear some of my concerns, hope you’ll find some time to address these comments. Thanks for putting in some work here :)

@@ -211,9 +210,9 @@
const observer =
`_firestoreUpdateBinding('${name}', ${args.join(',')})`
this._createMethodObserver(observer);
} else {
this._firestoreUpdateBinding(name, ...args.map(x => this[x]));
Copy link
Contributor

Choose a reason for hiding this comment

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

There are two issues here:

  • this won’t work if the values of properties are set before we create a method observer
  • even if it would work, we have a guarantee that args will have no items, so mapping over this is unnecessary

Copy link
Author

@Westbrook Westbrook Feb 24, 2018

Choose a reason for hiding this comment

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

this won’t work if the values of properties are set before we create a method observer

I can see how this might be an issue, but I'm not sure that the timing requisite of such a reality is possible with the code order of a Polymer component. Can you point me to how you envisioned that being possible?

even if it would work, we have a guarantee that args will have no items, so mapping over this is unnecessary

Good point, I can clean that up when you think this is going towards a solution.

}

connectedCallback() {
super.connectedCallback();
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask why did you decide to move it here? What’s the reasoning behind it?

Copy link
Author

Choose a reason for hiding this comment

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

I mainly stole these two lines from #279, but the goal is to not have an issue like this: https://receptive-kitten.glitch.me/ I can look into what this is actually fixing soon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it actually needed to fix it? I can’t really see why would it help.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, finally got into this part more. It helps because if you allow super.connectedCallback() to run, then the code at https://github.com/firebase/polymerfire/blob/master/firebase-app.html#L123 will also be allowed to fully run, which means that the app will have actually connected to Firebase. It's possible that you suggestions on moving some of the this.db stuff out to the scope might address this as well. However, were that not so, maybe this is a use case for https://www.polymer-project.org/2.0/docs/api/namespaces/Polymer.RenderStatus#function-Polymer.RenderStatus.afterNextRender ?

}

connectedCallback() {
super.connectedCallback();
this.db = this.constructor.db || firebase.firestore();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ever have this.constructor.db defined? I think we could improve it even further:

  • actually lazy-require Firestore (if binding is called with all props defined, line 231?)
  • introduce a helper method for this
  • we could move it out of the mixin entirely (to the scope), unless we want to give an access to db prop, then we would need to make some docs for it

Copy link
Author

Choose a reason for hiding this comment

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

Do we ever have this.constructor.db defined?

Doesn't seem so...copy pasta.

actually lazy-require Firestore (if binding is called with all props defined, line 231?)

Yes a this.db = this.db || firebase.firestore(); there would seem sufficient.

we could move it out of the mixin entirely (to the scope), unless we want to give an access to db prop, then we would need to make some docs for it

I'm not quite sure I follow.

Copy link
Contributor

@merlinnot merlinnot left a comment

Choose a reason for hiding this comment

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

Oops, I meant to comment, not approve :) Mobile device, sorry.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@Westbrook
Copy link
Author

Also, I wondered if you might have some thoughts on how to test this. Possibly there are appropriate steps towards getting Firestore permisions on https://polymerfire-test.firebaseio.com like some of these other steps? Seems like mocking the interactions here would be...arduous...unless, that is, there were already a mocking library for the Firebase libraries, 😱⁉️

But, make the `_createMethodObserver` after so that the first time doesn't loop back into `_firestoreUpdateBinding` a second time when `_firestoreUnlisten` zeros out the properties.
@@ -205,15 +204,14 @@
this._firestoreProps[name] = config;

const args = config.props.concat(config.observes);
this._firestoreUpdateBinding(name, ...args.map(x => this[x]));
Copy link
Author

Choose a reason for hiding this comment

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

Thought more about the issue at hand here, and it wasn't so much that I didn't want this called in every _firestoreBind call, but that I didn't want it called after the _createMethodObserver had been created because it meant that when _firestoreUnlisten was called as part of the initial _firestoreUpdateBinding call would immediately trigger a second call to _firestoreUpdateBinding because of the zeroing out of the value for the property being bound to at line 251.

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at how the observer is created: https://github.com/firebase/polymerfire/pull/327/files#diff-ed26b76bc80e40b84633288e109db714R211

The name is passed in quotes, just to indicate for which property arguments have changed. It does not observe the property itself.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. I didn't follow that through line deep enough.

With the code ordered this way, the initial properties flush of the args is able to be passed before _firestoreUnlisten makes the call to setProperties, which is why it's not called twice at runtime. This is due to the initial call to _flushProperties not occurring till https://github.com/Polymer/polymer/blob/master/lib/mixins/property-effects.html#L1661

Not sure if there is a more idiomatic way of achieving the timing we need here.

If you can't add a listener till all `propArgsReady && observesArgsReady` no need to remove the previous listener till then. Create the DB then too.
@@ -229,6 +225,8 @@
observesArgs.length === config.observes.length;

if (propArgsReady && observesArgsReady) {
this._firestoreUnlisten(name);
this.db = this.db || firebase.firestore();
Copy link
Author

Choose a reason for hiding this comment

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

Made some moves here to get this less likely to happen on first run.

@merlinnot I see what you're saying now about possibly moving db to the scope. I think the reason it's used like this is to mimic the way firebase-database-behavior.html actually adds db to the properties getter https://github.com/firebase/polymerfire/blob/master/firebase-database-behavior.html#L20 not sure if there's benefits to doing that or not here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see. I think we might want to rename this property, but I’ll make an issue for this, let’s not do it as a part of this PR.

Original (but broken) approach using constructor had a good intention of reusing the same firestore instance. What we should do IMO is:

  • create a variable in the scope to cache firestore instance
  • create a getter in the class which would assign a value to this variable as a side-effect if it’s undefined, than return it.
  • use this variable directly within the class methods to avoid calling the getters logic (performance)
  • make sure we initially assign it as late as possible (we might want to extract “assign if undefined”) logic to a helper method to reuse it in the getter and for initial assignment

Does it sound right?

Copy link
Author

Choose a reason for hiding this comment

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

I think that makes sense. I'm not quite sure I understand how to achieve what you're talking about in the last two bullets, so I can't say whether I think it's overkill or not. Would this look something like:

if (typeof Polymer === 'undefined') {
  throw new Error('Polymer.FirestoreMixin must be imported after Polymer itself.');
}
{
  let DB;

  Polymer.FirestoreMixin = parent =>
    get db() {
      DB = DB || firebase.firestore();
      return DB;
    }
    initDB() {
      DB ? return : this.db;
    }
    _firestoreUpdateBinding(name, ...args) {
      // ...
      if (propArgsReady && observesArgsReady) {
        this._firestoreUnlisten(name);
        initDB();
        // ...
        let ref = DB[config.type](collPath);
        // ...
      }
      // ...
    }
}

If so, I'm not completely sure how the "assign if undefined" logic in initDB would add any performance benefit over calling this.db directly. Also, correct me if I'm wrong, but doesn't this also assume that every instance of this mixin is then attached to the same Firestore remote? I've not spent much time attaching things to scope like this, so this is great learning for me.

@merlinnot
Copy link
Contributor

I’ve actually started to work on such library, but it will probably take some serious time and I’ll probably try to reach out to some googler soon to make sure they’re not working on this ATM so my time won’t go to waiste (@laurenzlong ?) 😃

Anyways, I’ll be working on tests for this on February 27th probably, so you might want to wait couple of days and just build on it. Guys from firestore-js-sdk team are actually using the real db to run tests, so... I’ll most probably do the same and just call it “integration tests” so I won’t feel bad with myself 😜

Allows listening to be unbound if one of the propArgs becomes un"ready" later in the lifecycle.
@tjmonsi
Copy link
Collaborator

tjmonsi commented Mar 6, 2018

@merlinnot is this ok to be merged on your branch now?

@merlinnot
Copy link
Contributor

Nope, unfortunately. I had some hope that I'll find some time over the weekend to write tests, reproduce this issue and fix the core problem here.

Sorry, but I can't approve it until we have some tests for this (which I'll try to write myself) + one of the changes is most definitely invalid.

@tjmonsi
Copy link
Collaborator

tjmonsi commented Jun 3, 2018

@merlinnot any updates on this? is it ok to merge to Firestore before I merge this firestore branch to master?

@merlinnot
Copy link
Contributor

Hey, I went through the changes once again and I’m simply not sure.

I have a few thoughts tho:

  1. This PR changes both database and Firestore, we can split it into two separate ones so it’s easier to review.
  2. I won’t be ok with any changes for Firestore until we have proper tests. The code is too complicated for this. I’ve tested the current polymerfire branch in production for months, it works fine, but I no longer use it since all of my projects are now upgraded to Lit + Redux + Saga
  3. I’m not convinced if we should actually release it if there is no one to take a proper care of it (the whole repo actually)
  4. I have a branch with some tests and minor refactoring which probably solves the issue with registration of Firestore, but it’s not in a finished state, lacks many tests and I won’t have time in a near future to work on it, especially when I’m busy working on some stuff in Firestore itself, like this: Added ES module bundle to the auth package firebase/firebase-js-sdk#819. It would be great if someone could pick up my work on testing first, before we (possibly) merge this PR.
  5. I feel like there needs to be a decision on what to do with Polymerfire. Things are kinda unclear right now, I feel like a community using Firebase with Polymer 1/2 is rather small and now, in the age of Lit and Polymer 3 (which is slowly dying too...), it will get even smaller. Maybe it should be simply deprecated?

We should probably move this discussion elsewhere, maybe at least create an issue. Let me know what you think.

@merlinnot merlinnot mentioned this pull request Jun 7, 2018
@tjmonsi
Copy link
Collaborator

tjmonsi commented Jun 8, 2018

@merlinnot #353 we can discuss it here

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.

7 participants