rocicorp / replicache-nextjs

Generic Replicache backend for Next.js
Apache License 2.0
21 stars 8 forks source link

Add ReplicacheProvider component #11

Closed xvvvyz closed 2 years ago

aboodman commented 2 years ago

I landed all of these except the context one here: https://github.com/rocicorp/replicache-nextjs/pull/12.

For the context one, I want to get a few more people on the teams' opinion, but I think it makes sense.

grgbkr commented 2 years ago

Nice work, thanks for this contribution.

@cesara @grgbkr - what do you think of this change? The idea is that you can just use useReplicache() without having to pass Replicache down through the props. I don't personally like this as much but I think it's more common in React-land, and am fully happy to accept that I'm old-fashioned.

Context: https://discord.com/channels/830183651022471199/1005896465601810554/1005898603136557106

It's a nice add on, but my preference would be for useReplicache() to remain the way it is today. Then ReplicacheProvider can be built on top of useReplicache(). Then places that want to retrieve Replicache via context would do: const rep = useContext(ReplicacheContext);

I prefer this because

  1. useReplicache still exists for those that prefer prop-drilling.
  2. useContext(ReplicacheContext) makes it clearer the Replicache instance is coming from context, tying it back more explicitly to the ReplicacheProvider.
xvvvyz commented 2 years ago

That approach makes sense to me @grgbkr.

How do we feel about updating the current useReplicache arguments to reflect ReplicacheOptions (like I've done to the new ReplicacheProvider component? i.e you could call useReplicache({ name: 'foo', mutators: ..., schemaVersion: '1', ...anyOtherConfigOptionsYouWantToChange) vs passing only spaceID & mutators.

aboodman commented 2 years ago

How do we feel about updating the current useReplicache arguments to reflect ReplicacheOptions

That part is a clear win I think. I like the way you've done it with Omit.

grgbkr commented 2 years ago

How do we feel about updating the current useReplicache arguments to reflect ReplicacheOptions

That part is a clear win I think. I like the way you've done it with Omit.

Agree.