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

New API: <Firebase /> with query and render props #49

Open
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@einarlove
Member

einarlove commented Oct 3, 2017

New feature: <Firebase /> component

This will add the capability to dynamicly change the subscriptions on render.
Inspired by Never Write Another HoC, a talk by @mjackson.

This will enable:

const Header ({ userId }) => (
  <header>
    <Logo />
    <Firebase
      query={(ref, app) => ({ 
        username: `users/${userId}/name`,
        logout: () => app.auth().signOut(),
      })}
      render={({ username, logout }) => 
        <h1>Welcome {username}</h1>
        <button onClick={logout}>Logout</button>
      }
    />
  </header>
)

const Todos = () => (
  <Firebase query="todos" render={todos => todos &&
    <ul>
      {todos.map(todo => <li>{todo}</li>)}
    </ul>
  } />
)

const Toggle = () => 
  <Firebase
    query={ref => ({
      toggle: 'toggle',
      setToggle: toggle => ref('toggle', toggle)
    })}
    render={({ toggle, setToggle }) =>
      <input
        type="checkbox"
        value={toggle}
        onChange={event => setToggle(event.target.checked)}
      />
    }
  />

Before merge

  • All tests should pass. Right now they expect FirebaseConnect to have access to the subscriptionState etc.
  • New tests for Firebase.js
  • Documentation in readme
Show outdated Hide outdated src/Firebase.js
Show outdated Hide outdated src/Firebase.js
Show outdated Hide outdated src/Firebase.js
@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 4, 2017

Member

Maybe we should stop supprting action handlers aswell and rather pass ref and firebaseApp in the render props.

const Toggle = () => 
  <Firebase
    query="toggle"
    render={(toggle, ref) =>
      <input
        type="checkbox"
        value={toggle}
        onChange={event => ref('toggle').set(event.target.checked)}
      />
    }
  />

Therefore query only consists of subscriptions and more fitting for the name.

Member

einarlove commented Oct 4, 2017

Maybe we should stop supprting action handlers aswell and rather pass ref and firebaseApp in the render props.

const Toggle = () => 
  <Firebase
    query="toggle"
    render={(toggle, ref) =>
      <input
        type="checkbox"
        value={toggle}
        onChange={event => ref('toggle').set(event.target.checked)}
      />
    }
  />

Therefore query only consists of subscriptions and more fitting for the name.

@simenbrekken

This comment has been minimized.

Show comment
Hide comment
@simenbrekken

simenbrekken Oct 4, 2017

Contributor

I really like the ref as second argument approach. Let's just make sure we cover all the existing use-cases in tests.

Contributor

simenbrekken commented Oct 4, 2017

I really like the ref as second argument approach. Let's just make sure we cover all the existing use-cases in tests.

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 4, 2017

Member

Published under prerelease 2.7.0-0 to be testet
npm i react-firebase@2.2.7-0

Member

einarlove commented Oct 4, 2017

Published under prerelease 2.7.0-0 to be testet
npm i react-firebase@2.2.7-0

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 4, 2017

Member

I think there should be a way to use this knowing if the date exists or not.

If you do this:

<Route path="tasks/:id" render={({ match }) =>
    <Firebase
      query={`tasks/${math.params.id}`}
      render={task => {
        if (!task) return <Redirect to="tasks" />

        return <Task {...task} />
      }}
    />
  }
/>

This will redirect immediately because it won't have a todo before firebase is connected. There is also no way to know if when a redirect should occur.

I would propose to not render before connected to firebase, or make it optional by making a prop like renderWhenConnected or similar. An alternative is to split the behavior into render and children prop like react-router. Render is called if connected to firebase, children is always called.

Member

einarlove commented Oct 4, 2017

I think there should be a way to use this knowing if the date exists or not.

If you do this:

<Route path="tasks/:id" render={({ match }) =>
    <Firebase
      query={`tasks/${math.params.id}`}
      render={task => {
        if (!task) return <Redirect to="tasks" />

        return <Task {...task} />
      }}
    />
  }
/>

This will redirect immediately because it won't have a todo before firebase is connected. There is also no way to know if when a redirect should occur.

I would propose to not render before connected to firebase, or make it optional by making a prop like renderWhenConnected or similar. An alternative is to split the behavior into render and children prop like react-router. Render is called if connected to firebase, children is always called.

const value = containsOrderBy ? mapSnapshotToValue(snapshot) : snapshot.val()
this.setState(prevState => ({
connected: true,

This comment has been minimized.

@einarlove

einarlove Oct 5, 2017

Member

If you query to multiple subscriptions like { toods: 'todos', user: `users/${userId}`} can we be sure they are "fetched" at the same time on first connect, @simenbrekken?

@einarlove

einarlove Oct 5, 2017

Member

If you query to multiple subscriptions like { toods: 'todos', user: `users/${userId}`} can we be sure they are "fetched" at the same time on first connect, @simenbrekken?

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Oct 11, 2017

Member

Drop render || children API and instead render only when connected and always if the always prop or similar is true.

ReactTraining/react-router#5583

Member

einarlove commented Oct 11, 2017

Drop render || children API and instead render only when connected and always if the always prop or similar is true.

ReactTraining/react-router#5583

@simenbrekken

This comment has been minimized.

Show comment
Hide comment
@simenbrekken

simenbrekken Oct 12, 2017

Contributor

@einarlove Agreed, or borrow terminology from GraphQL and set deferred if you'd like to render even if no data is present yet.

Contributor

simenbrekken commented Oct 12, 2017

@einarlove Agreed, or borrow terminology from GraphQL and set deferred if you'd like to render even if no data is present yet.

@sw-yx

This comment has been minimized.

Show comment
Hide comment
@sw-yx

sw-yx Jan 9, 2018

just saw this. whats the status?

sw-yx commented Jan 9, 2018

just saw this. whats the status?

@einarlove

This comment has been minimized.

Show comment
Hide comment
@einarlove

einarlove Jan 9, 2018

Member

Cold turkey. We currently are using projects with firebase on the server not the browser, so this has been neglected a bit. We are fan of the idea and want it complete it. More likely that we have time to finish this PR in february.

If anyone want to complete it submit a new pull request forked from this one.

Member

einarlove commented Jan 9, 2018

Cold turkey. We currently are using projects with firebase on the server not the browser, so this has been neglected a bit. We are fan of the idea and want it complete it. More likely that we have time to finish this PR in february.

If anyone want to complete it submit a new pull request forked from this one.

@sw-yx

This comment has been minimized.

Show comment
Hide comment
@sw-yx

sw-yx Jan 9, 2018

Thanks Einar. I might actually do this. still need some time to understand the code but i have been using the main react-firebase very well so far and want to give back. i also want to enable firestore on this (i see you have another issue for that)

sw-yx commented Jan 9, 2018

Thanks Einar. I might actually do this. still need some time to understand the code but i have been using the main react-firebase very well so far and want to give back. i also want to enable firestore on this (i see you have another issue for that)

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