- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 218
 
feat: writeDefaults #1159
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
base: next
Are you sure you want to change the base?
feat: writeDefaults #1159
Conversation
| 
           @TkDodo is attempting to deploy a commit to the 47ng Team on Vercel. A member of the Team first needs to authorize it.  | 
    
          commit:   | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I’m still seeing an additional re-render after writing the default to the url, even though we now bail out after calling setState(v => v).
I think that’s because the component still has null in its internal cache, and the useEffect updates it to the initial value.
This is not ideal because it will always happen when writeDefaults is set, even if you already have a value in the url.
Maybe you have a better idea how to write a value to the url without triggering any subscribers ?
        
          
                packages/nuqs/src/useQueryStates.ts
              
                Outdated
          
        
      | // effect to write defaults to the url on mount | ||
| useEffect(() => { | ||
| if (optionWriteDefaults || anyParserWriteDefaults) { | ||
| void update(s => s) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what updates the url and puts the defaults in there. Should we make this force to history: 'replace' ? I don't see how a push would make any sense here because if you try to navigate back, it would just automatically push the default in there again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah writeDefaults should use replace, that makes more sense.
As for "not triggering any subscribers" in #1159 (review), that depends on the frameworks, but generally when the URL changes, their useSearchParams API or equivalent will re-render at least once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are fine with the tradeoff, I am too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about at least bailing out of writing to the url if the url is empty and we want to write the defaultValue to it, but then again, scheduling makes this tricky: what if there’s another write scheduled, then we can’t just opt-out of this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like maybe setState(v => v) is too high-level an approach for this, maybe this needs to be better integrated with the throttle queue state.
The throttle queue works in two phases:
- Collecting pushes (in either the same event loop tick for sync batching, or until the throttle time expires if there has been a flush recently), which merges update requests into a map as it goes along.
 - A request for flush is made, which schedules it according to the queue's timing state (if enough time has passed, it gets flushed on next tick, otherwise at the end of the throttle timeout).
 
The debounce queues also offload their output to the throttle queue, so all updates that need to reach the URL go through the same pipeline.
The optimistic state and returned Promise flows the other way around, from the throttle queue to the debounce controller.
We could read from the adapter's getSearchParamsSnapshot for the "unoptimistic" view of what the URL is like for the router, and read the queued updates directly from the debounce controller (like useQueuedQueries, but in a non-hook, one-shot way), then decide whether or not to push+flush the update.
| * Set it to `false` to keep backwards-compatiblity when the default value | ||
| * changes (prefer explicit URLs whose meaning don't change). | ||
| * | ||
| * @deprecated use `writeDefaults` instead | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: we should add a migration note here that the boolean logic is inverted: clearOnDefault: false → writeDefaults: true.
| const anyParserWriteDefaults = Object.values(keyMap).some( | ||
| v => v.writeDefaults | ||
| ) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What happens in the following (albeit contrived) scenario?
const [dynamic, setDynamic] = useState(false)
useQueryStates({
  a: parseAsString.withDefault('a').withOptions({ writeDefaults: false }),
  b: parseAsString.withDefault('b').withOptions({ writeDefaults: dynamic })
})
// Then toggle dynamicI would expect a never to have its default being applied to the URL (as it's asking not to), but it feels like dynamic becoming true would trigger the effect and write them both.
No description provided.