-
Notifications
You must be signed in to change notification settings - Fork 929
refactor(database): Add typescript database implementation [Part 2/3] #62
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
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
4a0df56
refactor(database): add typescript implementation
jshcrowthe dcf2f5c
refactor(database): update build process to include database.ts
jshcrowthe 7e609d6
refactor(*): refactor environment builds to be based on separate .ts …
jshcrowthe e72bfbe
WIP: patch database code in nodeJS
jshcrowthe 2f6a406
refactor(database): classes for typescript database implementation (#55)
jsayol cf357d0
fix(database): Add missing "typeof" (#74)
jsayol 61b8a48
WIP: fixes from @schmidt-sebastian's review
jshcrowthe 71e4440
WIP: fix: TS Build error
jshcrowthe 25aeeed
fix(database): fix issue with missing repo method
jshcrowthe 61c8d25
WIP: review adjustments #1
jshcrowthe a01ff31
WIP: review comments #2
jshcrowthe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
/** | ||
* Copyright 2017 Google Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
import firebase from './app'; | ||
import { FirebaseApp, FirebaseNamespace } from "./app/firebase_app"; | ||
import { Database } from "./database/api/Database"; | ||
import { Query } from "./database/api/Query"; | ||
import { Reference } from "./database/api/Reference"; | ||
import { enableLogging } from "./database/core/util/util"; | ||
import { RepoManager } from "./database/core/RepoManager"; | ||
import * as INTERNAL from './database/api/internal'; | ||
import * as TEST_ACCESS from './database/api/test_access'; | ||
import { isNodeSdk } from "./utils/environment"; | ||
|
||
export function registerDatabase(instance) { | ||
// Register the Database Service with the 'firebase' namespace. | ||
const namespace = instance.INTERNAL.registerService( | ||
'database', | ||
app => RepoManager.getInstance().databaseFromApp(app), | ||
// firebase.database namespace properties | ||
{ | ||
Reference, | ||
Query, | ||
Database, | ||
enableLogging, | ||
INTERNAL, | ||
ServerValue: Database.ServerValue, | ||
TEST_ACCESS | ||
} | ||
); | ||
|
||
if (isNodeSdk()) { | ||
module.exports = namespace; | ||
} | ||
} | ||
|
||
/** | ||
* Extensions to the FirebaseApp and FirebaseNamespaces interfaces | ||
*/ | ||
declare module './app/firebase_app' { | ||
interface FirebaseApp { | ||
database?(): Database | ||
} | ||
} | ||
|
||
declare module './app/firebase_app' { | ||
interface FirebaseNamespace { | ||
database?(app: FirebaseApp): Database | ||
} | ||
} | ||
|
||
registerDatabase(firebase); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
import { validateArgCount, validateCallback } from '../../utils/validation'; | ||
import { validatePathString } from '../core/util/validation'; | ||
import { Path } from '../core/util/Path'; | ||
import { PRIORITY_INDEX } from '../core/snap/indexes/PriorityIndex'; | ||
import { Node } from '../core/snap/Node'; | ||
import { Reference } from './Reference'; | ||
import { Index } from '../core/snap/indexes/Index'; | ||
import { ChildrenNode } from '../core/snap/ChildrenNode'; | ||
|
||
/** | ||
* Class representing a firebase data snapshot. It wraps a SnapshotNode and | ||
* surfaces the public methods (val, forEach, etc.) we want to expose. | ||
*/ | ||
export class DataSnapshot { | ||
/** | ||
* @param {!Node} node_ A SnapshotNode to wrap. | ||
* @param {!Reference} ref_ The ref of the location this snapshot came from. | ||
* @param {!Index} index_ The iteration order for this snapshot | ||
*/ | ||
constructor(private readonly node_: Node, | ||
private readonly ref_: Reference, | ||
private readonly index_: Index) { | ||
} | ||
|
||
/** | ||
* Retrieves the snapshot contents as JSON. Returns null if the snapshot is | ||
* empty. | ||
* | ||
* @return {*} JSON representation of the DataSnapshot contents, or null if empty. | ||
*/ | ||
val(): any { | ||
validateArgCount('DataSnapshot.val', 0, 0, arguments.length); | ||
return this.node_.val(); | ||
} | ||
|
||
/** | ||
* Returns the snapshot contents as JSON, including priorities of node. Suitable for exporting | ||
* the entire node contents. | ||
* @return {*} JSON representation of the DataSnapshot contents, or null if empty. | ||
*/ | ||
exportVal(): any { | ||
validateArgCount('DataSnapshot.exportVal', 0, 0, arguments.length); | ||
return this.node_.val(true); | ||
} | ||
|
||
// Do not create public documentation. This is intended to make JSON serialization work but is otherwise unnecessary | ||
// for end-users | ||
toJSON(): any { | ||
// Optional spacer argument is unnecessary because we're depending on recursion rather than stringifying the content | ||
validateArgCount('DataSnapshot.toJSON', 0, 1, arguments.length); | ||
return this.exportVal(); | ||
} | ||
|
||
/** | ||
* Returns whether the snapshot contains a non-null value. | ||
* | ||
* @return {boolean} Whether the snapshot contains a non-null value, or is empty. | ||
*/ | ||
exists(): boolean { | ||
validateArgCount('DataSnapshot.exists', 0, 0, arguments.length); | ||
return !this.node_.isEmpty(); | ||
} | ||
|
||
/** | ||
* Returns a DataSnapshot of the specified child node's contents. | ||
* | ||
* @param {!string} childPathString Path to a child. | ||
* @return {!DataSnapshot} DataSnapshot for child node. | ||
*/ | ||
child(childPathString: string): DataSnapshot { | ||
validateArgCount('DataSnapshot.child', 0, 1, arguments.length); | ||
// Ensure the childPath is a string (can be a number) | ||
childPathString = String(childPathString); | ||
validatePathString('DataSnapshot.child', 1, childPathString, false); | ||
|
||
const childPath = new Path(childPathString); | ||
const childRef = this.ref_.child(childPath); | ||
return new DataSnapshot(this.node_.getChild(childPath), childRef, PRIORITY_INDEX); | ||
} | ||
|
||
/** | ||
* Returns whether the snapshot contains a child at the specified path. | ||
* | ||
* @param {!string} childPathString Path to a child. | ||
* @return {boolean} Whether the child exists. | ||
*/ | ||
hasChild(childPathString: string): boolean { | ||
validateArgCount('DataSnapshot.hasChild', 1, 1, arguments.length); | ||
validatePathString('DataSnapshot.hasChild', 1, childPathString, false); | ||
|
||
const childPath = new Path(childPathString); | ||
return !this.node_.getChild(childPath).isEmpty(); | ||
} | ||
|
||
/** | ||
* Returns the priority of the object, or null if no priority was set. | ||
* | ||
* @return {string|number|null} The priority. | ||
*/ | ||
getPriority(): string | number | null { | ||
validateArgCount('DataSnapshot.getPriority', 0, 0, arguments.length); | ||
|
||
// typecast here because we never return deferred values or internal priorities (MAX_PRIORITY) | ||
return /**@type {string|number|null} */ <string | number | null>(this.node_.getPriority().val()); | ||
} | ||
|
||
/** | ||
* Iterates through child nodes and calls the specified action for each one. | ||
* | ||
* @param {function(!DataSnapshot)} action Callback function to be called | ||
* for each child. | ||
* @return {boolean} True if forEach was canceled by action returning true for | ||
* one of the child nodes. | ||
*/ | ||
forEach(action: (d: DataSnapshot) => any): boolean { | ||
validateArgCount('DataSnapshot.forEach', 1, 1, arguments.length); | ||
validateCallback('DataSnapshot.forEach', 1, action, false); | ||
|
||
if (this.node_.isLeafNode()) | ||
return false; | ||
|
||
const childrenNode = /**@type {ChildrenNode} */ <ChildrenNode>(this.node_); | ||
// Sanitize the return value to a boolean. ChildrenNode.forEachChild has a weird return type... | ||
return !!childrenNode.forEachChild(this.index_, (key, node) => { | ||
return action(new DataSnapshot(node, this.ref_.child(key), PRIORITY_INDEX)); | ||
}); | ||
} | ||
|
||
/** | ||
* Returns whether this DataSnapshot has children. | ||
* @return {boolean} True if the DataSnapshot contains 1 or more child nodes. | ||
*/ | ||
hasChildren(): boolean { | ||
validateArgCount('DataSnapshot.hasChildren', 0, 0, arguments.length); | ||
|
||
if (this.node_.isLeafNode()) | ||
return false; | ||
else | ||
return !this.node_.isEmpty(); | ||
} | ||
|
||
get key() { | ||
return this.ref_.getKey(); | ||
} | ||
|
||
/** | ||
* Returns the number of children for this DataSnapshot. | ||
* @return {number} The number of children that this DataSnapshot contains. | ||
*/ | ||
numChildren(): number { | ||
validateArgCount('DataSnapshot.numChildren', 0, 0, arguments.length); | ||
|
||
return this.node_.numChildren(); | ||
} | ||
|
||
/** | ||
* @return {Reference} The Firebase reference for the location this snapshot's data came from. | ||
*/ | ||
getRef(): Reference { | ||
validateArgCount('DataSnapshot.ref', 0, 0, arguments.length); | ||
|
||
return this.ref_; | ||
} | ||
|
||
get ref() { | ||
return this.getRef(); | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
As discussed over chat, we should figure out if we want to keep the type annotations in our comment or if it is sufficient to use the types from TypeScript.
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.
Just chiming in since I was thinking about this the other day. Type annotations in the comments only make sense when using a tool like Closure compiler to do type checking, but it's not necessary anymore with TypeScript. Besides, I bet the types in the comments won't always be updated when the code changes so eventually it might do more harm than good.
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'm happy w/ defaulting to typescripts types. Just was trying to keep the delta small where possible.
That said, I think I'm going to merge #66 as I've been going through that and it addresses many of the type issues you've mentioned.
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.
In addition I agree with @jsayol, that the divergence in types is probably a maintenance burden going forward. Let's consolidate after we've validated the code is good.