Skip to content
This repository was archived by the owner on May 19, 2025. It is now read-only.

added an "exists" attribute to differentiate whether we have data or not#177

Closed
christophe-g wants to merge 7 commits intoFirebaseExtended:masterfrom
PolymerEl:addExistsAttribute
Closed

added an "exists" attribute to differentiate whether we have data or not#177
christophe-g wants to merge 7 commits intoFirebaseExtended:masterfrom
PolymerEl:addExistsAttribute

Conversation

@christophe-g
Copy link
Copy Markdown
Contributor

follows discussions #33.

Comment thread firebase-query.html Outdated
this.async(function() {
this.syncToMemory(function() {
this.splice('data', this.__indexFromKey(key), 1);
if (this.data.length === 0) {
Copy link
Copy Markdown

@phrohdoh phrohdoh Jan 31, 2017

Choose a reason for hiding this comment

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

Doesn't this imply the data in memory does not reflect what is in Firebase?

If that is the case then this isn't helpfully in the case of [] which could very well exist in either location and be valid.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks,
http://stackoverflow.com/questions/15408416/how-to-handle-empty-arrays-in-firebase : "there is no way to force empty array into firebase".

And yes, data in memory does not always reflect what is in Firebase - see discussion #177 (zeroValue).

@mbleigh
Copy link
Copy Markdown
Contributor

mbleigh commented Feb 4, 2017

Would you consider adding a statusKnown property to this PR in addition to exists? Right now you're having to use a three-value boolean (null, false, true) to track state which feels off to me. Ideally I'd prefer statusKnown which indicates whether data has ever been received for the current path, and exists which indicates whether it's null.

@christophe-g
Copy link
Copy Markdown
Contributor Author

Thanks a lot for looking at this and merging #166 and #167.

Makes perfect sense to add a statusKnown attribute. Should land shortly.

@christophe-g
Copy link
Copy Markdown
Contributor Author

Landed

@andrewspy
Copy link
Copy Markdown

Any update on merging statusKnown attribute?

@christophe-g
Copy link
Copy Markdown
Contributor Author

Seems the codebase diverted quite a bit and this PR would need to be rebased against master. I'll do that once #213 is merged.
C.

@christophe-g
Copy link
Copy Markdown
Contributor Author

christophe-g commented Jun 17, 2017

Closed by #234

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants