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.

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

Cannot change pusher or puller after initialization #685

Closed
aboodman opened this issue Nov 13, 2021 · 6 comments · Fixed by #696
Closed

Cannot change pusher or puller after initialization #685

aboodman opened this issue Nov 13, 2021 · 6 comments · Fixed by #696
Assignees

Comments

@aboodman
Copy link
Contributor

I think this is a regression in 8.0, but I'm not sure:

const r = new Replicache();
r.pusher = ...
r.puller = ...

The pusher and puller impls won't get called because we create a NullConnectionLoop at startup and it is never changed. I think it might work OK if you put a no-op pusher/puller in there instead though.

@aboodman
Copy link
Contributor Author

@grgbkr think this was fallout of some refactoring you did. Sorry there was no sufficient coverage to catch this.

@grgbkr
Copy link
Contributor

grgbkr commented Nov 15, 2021

Did not realize this was part of the API.

@aboodman What is the use case for changing pusher or puller after initialization? I'm sure I can fix this, but am wondering if its better to just remove this part of the API, and make these be provided at time of constructing Replicache.

@aboodman
Copy link
Contributor Author

I think we had a use case for changing some other configuration (maybe some of the rate limiting controls) and decided we may as well make the entire config interface mutable. I can't remember anything specific to pusher/puller -- maybe @arv does. I think we just thought the API was more consistent thing way.

I think it's clear that given pusher and puller are functions, you can clearly make them stubs and swap what they point to, so this isn't necessary.

I guess I don't have a strong opinion here. I do like removing things we don't have a clear need for as it constrains future development.

@arv
Copy link
Contributor

arv commented Nov 15, 2021

I went through all the options at one point thinking which ones must be readonly to keep semantics sane. Everything else was made read-write to allow flexibility.

#443

@grgbkr
Copy link
Contributor

grgbkr commented Nov 15, 2021

Let me see if i can support it without overly complicating the code.

@grgbkr
Copy link
Contributor

grgbkr commented Nov 16, 2021

This was easy enough to fix without changing the API.

grgbkr added a commit that referenced this issue Nov 16, 2021
…on are ignored if initially none set. (#696)

Also updates tests to cover these cases.

Fixes #685
arv pushed a commit that referenced this issue Nov 22, 2021
…on are ignored if initially none set. (#696)

Also updates tests to cover these cases.

Fixes #685
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants