-
Notifications
You must be signed in to change notification settings - Fork 142
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
base: firestore
Are you sure you want to change the base?
Firestore #327
Changes from 20 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,19 +203,17 @@ | |
this._firestoreProps[name] = config; | ||
|
||
const args = config.props.concat(config.observes); | ||
this._firestoreUpdateBinding(name, ...args.map(x => this[x])); | ||
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 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 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; | ||
|
@@ -229,6 +225,8 @@ | |
observesArgs.length === config.observes.length; | ||
|
||
if (propArgsReady && observesArgsReady) { | ||
this._firestoreUnlisten(name); | ||
this.db = this.db || firebase.firestore(); | ||
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. 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 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. 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
Does it sound right? 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. 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 so, I'm not completely sure how the "assign if undefined" logic in |
||
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.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 ?