Skip to content

Add support of nulled references as props of hooks #10

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No 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? No Sign in to your account

Merged
merged 2 commits into from
Apr 12, 2019

Conversation

neilor
Copy link
Contributor

@neilor neilor commented Mar 16, 2019

Developing using these hooks, I've been in front of situations that references cannot be mounted at the first component execution. As explained on (https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level)[React docs], hooks cannot be called inside as an if statement.

To avoid this, we can pass an invalid reference to the firebase hooks. But, if you have implemented the strongest security rules, you will receive a permission denied error.

So, we need to pass references optionally to solve all of these problems.

I've already added support by this PR!

Example of usage in a container that fits authenticated or not state:

export function useUsername() {
  const { user } = useAuthState(auth)
  const usernameRef = user
    ? db.child('profiles').child(user.uid).child('username')
    : undefined
  const { value: username, loading, error } = useDatabaseObject(usernameRef)
  return { username, loading, error }
}

@chrisbianca
Copy link
Contributor

@neilor Thanks for this PR. I can certainly understand the use case, I just want to put it through a few tests to check there haven't been any unexpected side effects. I'm hoping to do that at some point today and get this merged and released.

@neilor
Copy link
Contributor Author

neilor commented Mar 22, 2019

Nice, I've tested rtdb module a lot and it works fine for me. No permission denied errors and loading state works as expected, in my opinion.

I've published a version on my private npm from my fork in order to use in my app.

Can I help you with something more?

@tograd
Copy link

tograd commented Apr 4, 2019

I've run into this too and this was one of my potential solutions too. Another one is what about allowing a function that returns a query/reference | null

I've not done any tests or anything so maybe this solution is unusable for some good/obvious reason (I prefer it to the aforementioned one because of the brevity it allows and I don't think the performance of the function is of any bother, though speculative)

export function useUsername() {
  const { user } = useAuthState(auth)
  const { value: username, loading, error } = useDatabaseObject(() => {
    if (!user) return null;

    return db.child('profiles').child(user.uid).child('username');
  })

  return { username, loading, error }
}

And I also think both these solutions might earn a third state parameter like { value, loading, error, initializing} where "initializing" (or something else) is true while query is null? Though unsure if that has practical purpose beyond just using the loading variable

@nhagen
Copy link

nhagen commented Apr 11, 2019

I think a function is fine too, as long as we can pass null, but I'm not sure what benefit it provides if the function doesn't receive any dynamic parameters? Is that any different than just doing:

...
  const { user = null } useAuthState(auth);
  const document = user && db.child('profiles').child('user.uid').child('username');
  const { ... } = useDatabaseObject(document);
...

edit: I guess thats really not different than the first example in this thread. My question really is, is there any new functionality that this creates beyond a different ergonimic?

@neilor
Copy link
Contributor Author

neilor commented Apr 12, 2019

I've changed all of the hooks to support this nulled case. Behind the scenes, they only ignore calls with a null parameter, switching off the loading state and responding with null in values.

@neilor
Copy link
Contributor Author

neilor commented Apr 12, 2019

@chrisbianca What's blocking the publishment of a new version containing these changes?

@chrisbianca
Copy link
Contributor

Sorry, I've been completely snowed under. I'm going to merge these changes now and do some testing. I'll publish a new version once I've tested there's no unexpected side effects.

@chrisbianca chrisbianca merged commit 9f3c5a4 into CSFrequency:master Apr 12, 2019
const query: database.Query = ref.current;
const query: database.Query | null | undefined = ref.current;
if (!query) {
dispatch({ type: 'value' })
Copy link
Contributor

Choose a reason for hiding this comment

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

@neilor Currently this means that when no query is specified, useList will have an empty array as its value. Do you think this is correct? I'm torn as to whether this should be undefined or null as this better represents an absence of a value.

Copy link
Contributor Author

@neilor neilor Apr 14, 2019

Choose a reason for hiding this comment

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

Nice job! yes, I think too.

chrisbianca added a commit that referenced this pull request Apr 12, 2019
chrisbianca added a commit that referenced this pull request Apr 12, 2019
@neilor neilor deleted the feature/nullable branch April 14, 2019 03:15
@neilor
Copy link
Contributor Author

neilor commented Apr 14, 2019

@chrisbianca I've built a production app using your library with this covered case(nulled), published through my fork. I want to congratulate you for that amazing job, this simple and too much usable library!

PS: I and @raugfer have been made an experimental library that empowers the real-time database with offline schema validation, specifications(https://json-schema.org/), rules, types, and composed indexes by writing only a single manifest. Actually, this brings us robust production applications with a lot of productivity during development. Do you have interests to talk with us about it? Would be interesting for us to receive feedback and helping with the way to be a open source project.

@chrisbianca
Copy link
Contributor

This has now been released as part of the v1.2.0 release: https://github.com/CSFrequency/react-firebase-hooks/releases/tag/v1.2.0

@neilor Thanks for your contribution. I'd be more than happy to chat with you about your library - it sounds interesting!

No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants