WIP: RxFire #933

davideast · 2018-06-15T18:20:45Z

Discussion

RxJS + Firebase = 🔥🔥🔥

WIP. Will provide a deep description soon.

…s-sdk into rxfire-infra

jshcrowthe

Build Feedback

.gitignore could use some more comments explaining some of the rationale behind the .d.ts ignores
Delete .npmignore in favor of a pkg.files in the package.json
Why does packages/rxfire/index.ts exist?
Why does packages/rxfire/test/index.ts exist?
Set pkg.private in the package.json to true for now

Code Feedback

As a matter of convention should we prepending all of our streams w/ $?
We are using indexOf as Array.prototype.includes can we not do that (we have varying comparisons throughout the codebase for this)?

jshcrowthe · 2018-06-15T18:28:41Z

packages/rxfire/auth/package.json

@@ -0,0 +1,5 @@
+{
+  "name": "rxfire-auth",


Call this rxfire/auth

jshcrowthe · 2018-06-15T18:29:37Z

packages/rxfire/package.json

@@ -0,0 +1,69 @@
+{
+  "name": "rxfire",
+  "version": "0.0.2",


Just call this 0.0.0 for now

jshcrowthe · 2018-06-15T18:31:52Z

packages/rxfire/rollup.config.js

+/**
+ * Complete Package Builds
+ */
+const completeBuilds = [


Delete this, it's not needed.

jshcrowthe · 2018-06-15T18:32:06Z

packages/rxfire/rollup.config.js

+ */
+const GLOBAL_NAME = 'rxfire';
+
+const coreBuild = [


Delete this, not relevant.

jshcrowthe · 2018-06-15T18:34:26Z

packages/rxfire/rollup.config.js

+import storagePkg from './storage/package.json';
+import functionsPkg from './functions/package.json';
+
+const pkgsByName = {


Looks like you aren't compiling everything here.

jshcrowthe · 2018-06-15T19:05:33Z

packages/rxfire/firestore/collection/index.ts

+ * @param change
+ */
+function combineChange<T>(
+  combined: firestore.DocumentChange[],


Let's be more descriptive of what this actually is in this name (i.e. the full array of changes and a single change to process)

jshcrowthe · 2018-06-15T19:06:00Z

packages/rxfire/firestore/collection/index.ts

+ * @param combined
+ * @param change
+ */
+function combineChange<T>(


This function name isn't indicative of what is actually being done here, let's think of a new name?

jshcrowthe · 2018-06-15T19:10:26Z

packages/rxfire/firestore/collection/index.ts

+}
+
+/**
+ * Return a stream of document changes on a query. These results are in sort order.


This comment is inaccurate, we return a stream of DocumentSnapshots not changes.

jshcrowthe · 2018-06-15T19:19:13Z

packages/rxfire/firestore/collection/index.ts

+
+/**
+ * Create a stream of changes as they occur it time. This method is similar
+ * to stateChanges() but it collects each event in an array over time.


stateChanges doesn't exist?

it's now docChanges()

jshcrowthe · 2018-06-15T19:24:12Z

packages/rxfire/storage/index.ts

+export function putString(
+  ref: storage.Reference,
+  data: string,
+  format?: storage.StringFormat,


This isn't used in the body? Perhaps you meant ref.putString?

googlebot · 2018-06-15T19:51:11Z

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 or co-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.

…s-sdk into rxfire-infra

jshcrowthe

Couple more comments on the root package.json.

Rest is looking good.

jshcrowthe · 2018-06-26T21:20:21Z

packages/rxfire/package.json

+    "./firestore",
+    "./functions",
+    "./storage",
+    "./package.json",


Not needed, the root package.json is included by default.

jshcrowthe · 2018-06-26T21:25:17Z

packages/rxfire/package.json

@@ -67,6 +68,18 @@
  },
  "typings": "dist/index.d.ts",
  "files": [
-
+    "./auth",


Looks like for all of these dirs you need two things:

The package.json

The dist dir.

Can we not include source here and instead just do that?

jshcrowthe

Guess I wasn't fully clear, including the entire directory for auth, firestore, functions, and storage, is not necessary. Let's only include the package.json and the dist dir, inside each of those directories.

davideast added 13 commits May 23, 2018 15:13

rxfire initial setup

300a1a6

operators as external

cc5f15e

testing config. storage observables.

af4a601

functions observable

1676c7b

firestore

5661262

Firestore tests

3cdc3d7

[AUTOMATED]: Prettier Code Styling

28c3a11

[AUTOMATED]: License Headers

a7a49f6

fixed declaration problem

9d9769c

fix double emission in docChanges()

da2cfb7

[AUTOMATED]: Prettier Code Styling

cec2d57

comments

f4ea936

[AUTOMATED]: Prettier Code Styling

4503a55

davideast requested review from hiranya911 and jshcrowthe as code owners June 15, 2018 18:20

google-oss-bot added the needs-triage label Jun 15, 2018

Merge branch 'master' into rxfire-infra

3d1a53a

firebase deleted a comment from googlebot Jun 15, 2018

davideast added 2 commits June 15, 2018 12:37

firestore build

ddc24aa

Merge branch 'rxfire-infra' of https://github.com/firebase/firebase-j…

4cb0a99

…s-sdk into rxfire-infra

jshcrowthe suggested changes Jun 15, 2018

View reviewed changes

davideast added 2 commits June 15, 2018 13:49

josh comments

76a1124

[AUTOMATED]: Prettier Code Styling

065729c

davideast requested review from mikelehen, schmidt-sebastian and wilhuff as code owners June 15, 2018 19:51

davideast and others added 3 commits June 26, 2018 14:36

Merge branch 'master' into rxfire-infra

e87d98a

addressing Josh's comments.

85c9da4

Merge branch 'rxfire-infra' of https://github.com/firebase/firebase-j…

8887b51

…s-sdk into rxfire-infra

davideast added 2 commits June 26, 2018 15:17

forEach to for

ee8915d

[AUTOMATED]: Prettier Code Styling

07fb713

jshcrowthe suggested changes Jun 26, 2018

View reviewed changes

davideast added 2 commits June 26, 2018 15:29

updated comments

97ce638

package.json files

5831f83

jshcrowthe suggested changes Jun 26, 2018

View reviewed changes

davideast added 2 commits June 26, 2018 16:34

package.json files

dca768f

package.json files

22d6f13

jshcrowthe approved these changes Jun 26, 2018

View reviewed changes

Merge branch 'master' into rxfire-infra

fb759b4

davideast merged commit d134c39 into master Jun 26, 2018

davideast deleted the rxfire-infra branch June 26, 2018 23:35

arjunyel mentioned this pull request Jul 25, 2018

Direction of Polymerfire FirebaseExtended/polymerfire#353

Open

firebase locked and limited conversation to collaborators Oct 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: RxFire #933

WIP: RxFire #933

davideast commented Jun 15, 2018

jshcrowthe left a comment •

edited

Loading

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 26, 2018

jshcrowthe Jun 15, 2018

davideast Jun 15, 2018

jshcrowthe Jun 15, 2018

davideast Jun 15, 2018

googlebot commented Jun 15, 2018

jshcrowthe left a comment

jshcrowthe Jun 26, 2018

jshcrowthe Jun 26, 2018

jshcrowthe left a comment

WIP: RxFire #933

WIP: RxFire #933

Conversation

davideast commented Jun 15, 2018

Discussion

jshcrowthe left a comment • edited Loading

Choose a reason for hiding this comment

Build Feedback

Code Feedback

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jun 15, 2018

jshcrowthe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshcrowthe left a comment

Choose a reason for hiding this comment

jshcrowthe left a comment •

edited

Loading