Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented createAppContainer() and createAuthContainer() #123

Merged
merged 5 commits into from Apr 13, 2017
Merged

Implemented createAppContainer() and createAuthContainer() #123

merged 5 commits into from Apr 13, 2017

Conversation

jwngr
Copy link

@jwngr jwngr commented Feb 20, 2017 •
edited

Description

This implements two of the higher-order components that will make up ReactFire 2.0.0: createAuthContainer() and createAppContainer().

createAuthContainer() adds a user prop to the provided component that is updated every time the auth state is changed. You can change the name of the prop by passing an optional second argument

createAppContainer() adds a firebaseApp prop to the provided component. Using this in addition with createAuthContainer() allows you to listen for the auth state changes for the non-default Firebase app instance.

Other miscellaneous changes:

  • Replaced Mocha with Jest and made use of some of Jest's snapshot testing.
  • Replaced gulp with npm scripts.

Code sample

const ExampleComponentWithAuth = createAuthContainer(ExampleComponent);

// Advanced use cases
const ExampleComponentWithAuthForNonDefaultApp = createAppContainer(ExampleComponentWithAuth, someApp)
const OtherComponentWithAuthForNonDefaultApp = createAppContainer(ExampleComponentWithAuth, otherApp);

@jwngr jwngr requested a review from tstirrat February 20, 2017 23:00
@coveralls
Copy link

coveralls commented Feb 20, 2017 •
edited

Coverage Status

Changes Unknown when pulling 1b48747 on jw-reactfire-2 into ** on jw-es6**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 225e3f4 on jw-reactfire-2 into ** on jw-es6**.

1 similar comment
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 225e3f4 on jw-reactfire-2 into ** on jw-es6**.

@jwngr jwngr mentioned this pull request Feb 27, 2017
Closed
tstirrat
tstirrat approved these changes Mar 9, 2017
View reviewed changes
Copy link
Contributor

@tstirrat tstirrat left a comment

Choose a reason for hiding this comment

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

Added some notes, mostly nitpicks

package.json
"test": "gulp test",
"travis": "gulp"
"test": "npm run lint ; npm run build ; jest --coverage",
"lint": "eslint src/ tests/",
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we can use the node_modules/.bin/ binaries here so that they do not need global installation

Copy link
Author

Choose a reason for hiding this comment

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

They actually don't need a global install. I'm not sure if this is a newer npm thing, but anything with node_modules/.bin/ is addressable just by name in npm scripts.

src/createAuthContainer.js

let unsubscribe;

class CreateAuthContainer extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: It'd make more sense to me if this was just called AuthContainer, same goes for CreateAppContainer -> AppContainer

Copy link
Author

Choose a reason for hiding this comment

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

I'm conflicted and honestly not sure what the most correct thing to call this is. Let's chat about it in person tomorrow.

src/utils.js
*
* @return {boolean} Whether or not the provided input is a Firebase Database Reference.
*/
export function isDatabaseReference(input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't appear to be used.

Copy link
Author

Choose a reason for hiding this comment

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

Will be used in the next PR. If it's cool with you, I'll just keep it in this PR.

src/utils.js
* @return {boolean} Whether or not the provided input is either a Firebase Database Reference or
* a Firebase Database Query.
*/
export function isDatabaseReferenceOrQuery(input) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, doesn't appear to be used.

Copy link
Author

Choose a reason for hiding this comment

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

Ditto on the above.

@jwngr jwngr assigned jwngr and unassigned tstirrat Mar 30, 2017
@tstirrat
Copy link
Contributor

LGTM

@jwngr jwngr merged commit d4d58c9 into jw-es6 Apr 13, 2017
@jwngr jwngr deleted the jw-reactfire-2 branch April 13, 2017 19:13
@FirebaseExtended FirebaseExtended locked and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tstirrat tstirrat tstirrat approved these changes

Assignees

@jwngr jwngr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jwngr @coveralls @tstirrat