-
Notifications
You must be signed in to change notification settings - Fork 139
Firestore #327
base: firestore
Are you sure you want to change the base?
Firestore #327
Changes from all commits
844dcfd
5274aeb
91d7793
15f1c8a
f6df335
f107307
f6b0b14
58ed184
c8e301c
ac153f4
ab634d9
7290611
a7e445e
6ca00b6
ba6a8e8
7ea2df2
7ff8a37
a3f9653
63cbcaa
fd337f1
24e2d64
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,10 +169,10 @@ | |
|
|
||
| this._firestoreProps = {}; | ||
| this._firestoreListeners = {}; | ||
| this.db = this.constructor.db || firebase.firestore(); | ||
| } | ||
|
|
||
| connectedCallback() { | ||
| super.connectedCallback(); | ||
| if (this[CONNECTED_CALLBACK_TOKEN] !== true) { | ||
| this[CONNECTED_CALLBACK_TOKEN] = true; | ||
|
|
||
|
|
@@ -188,8 +188,6 @@ | |
| } | ||
| } | ||
| } | ||
|
|
||
| super.connectedCallback(); | ||
| } | ||
|
|
||
| _firestoreBind(name, options) { | ||
|
|
@@ -205,20 +203,18 @@ | |
| this._firestoreProps[name] = config; | ||
|
|
||
| const args = config.props.concat(config.observes); | ||
| this._firestoreUpdateBinding(name, ...args.map(x => this[x])); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Not sure if there is a more idiomatic way of achieving the timing we need here. |
||
| if (args.length > 0) { | ||
| // Create a method observer that will be called every time | ||
| // a templatized or observed property changes | ||
| const observer = | ||
| `_firestoreUpdateBinding('${name}', ${args.join(',')})` | ||
| this._createMethodObserver(observer); | ||
| } | ||
|
|
||
| this._firestoreUpdateBinding(name, ...args.map(x => this[x])); | ||
| } | ||
|
|
||
| _firestoreUpdateBinding(name, ...args) { | ||
| this._firestoreUnlisten(name); | ||
|
|
||
| const config = this._firestoreProps[name]; | ||
| const isDefined = (x) => x !== undefined; | ||
| const propArgs = args.slice(0, config.props.length).filter(isDefined); | ||
|
|
@@ -229,6 +225,7 @@ | |
| observesArgs.length === config.observes.length; | ||
|
|
||
| if (propArgsReady && observesArgsReady) { | ||
| this.db = this.db || firebase.firestore(); | ||
| const collPath = stitch(config.literals, propArgs); | ||
| const assigner = this._firestoreAssigner(name, config); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thethis.dbstuff 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 ?