-
Notifications
You must be signed in to change notification settings - Fork 642
[WIP] Tweak the API to handle initial state correctly #255
Conversation
No idea why some tests fail on Firefox with |
Note: most changes in |
07e4141
to
151154f
Compare
Why have the middleware at all? Structurally, this seems sound. Why not make it a store enhancer? Also, should the syncing default to opt in? If this works well, I'd want everyone to use it by default and have an option to disable for edge cases. |
Yeah store enhancer might be a nicer fit. I’ll give it a go. |
I'm in the middle of a big project at work, so I don't have a lot of time to think about this right now, but this looks like an interesting approach. Why have the named parameters at all though? Is there are reason we can just enable both by default and not make it configurable? The only reason for My only concern is API churn. I know that we want to find the right API and it's probably worth it, but I hate having to ask users to change again. There's already a feeling of too many moving parts with the various libraries, and I wish we could make it smoother somehow. This change only affects where you init everything, but you mentioned a bigger change in #248 (comment) that would affect everyone who reads from the state. I like the idea, but a lot of people probably aren't going to be happy about hiding the |
I don't mean this as a final API (especially regarding the named arguments etc). I also don't mind creating another library to handle that if you feel it's out of scope. It's just I feel that asking user not to subscribe to a part of their store is sketchy. Using middleware for stateful things is also sketchy (e.g. I think current version might be broken if you create two separate stores because it keeps shared state). The persistence support is one of the goals of this library, so I think #252 needs to be fixed, even if without revamping the API. |
Superseded by reduxjs/redux#1362. |
I know this is going to be controversial..
I think it’s wrong that the middleware dispatches during its creation. We probably won't allow it in next versions of Redux anyway.
The fact that this dispatch happens too early also caused #252. When the initial state is specified to the store and we “listen to replays”, we want the state to be the source of truth in the absence of user action. So if we load a location from localStorage and pass it into the initial state, whether with devtools or not, we want the URL to be set from it.
Since we need to extract a method to cause the initial dispatch anyway, I thought I might as well unite these two methods and turn them into options:
I know not everybody is a fan of named arguments so I’m very open to suggestions here. My reasoning for uniting these two methods was the fact that:
Therefore having to call one or two methods just to get the thing working seems like overkill. Additionally, named arguments explain what actually happens in very straightforward two-way binding terms, which is what this library actually is: two-way bindings from URL to a state field.
If you think this API sucks I’ll be happy to rewrite this to your API of choice. The primary purpose of this PR is still to fix #252, and I added a couple of new tests verifying that it is indeed now fixed.
This PR updates tests, example, and first part of the README. I am happy to update the API usage part when/if we agree on the API.