-
-
Notifications
You must be signed in to change notification settings - Fork 903
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
fix!: refactor v7 internal state and options logic, fixes #764 #779
Conversation
Co-authored-by: Robert Kieffer <[email protected]> Co-authored-by: Robin Pokorny <[email protected]>
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 really like it now! I think the trade offs are well selected. Good job 🎉
Maybe, in README we should write about the monotonicity and that providing any options means no guarantees of it. WDYT?
I'll update the README once this PR and #780 land. |
[#776, reopened as a new PR. No idea why it's including so many commits that are already on
main
]Putting this PR up as an alternative to #768 that addresses #764 and a few other concerns.
v7()
-with-options fromv7()
-without-options.seq
field rather than 31.@robinpokorny My apologies for closing #768. I wanted to merge it, but the switch to typescript and these other issues just made it easier to address in a new PR. I'll do my best to credit your work here.
... and if you could help review this, I'd appreciate it. I feel pretty good about this refactor but there's enough code churn that an extra set of eyes would be very helpful.
Where #764 is concerned, the heart of this PR is in this code, where the behavior very deliberately branches based on whether
options
are present.Note
options#seq
andoptions#msecs
.v1()
code would probably benefit from a similar refactoring, if for nothing else than to make the APIs consistent. I'll put up a PR for that shortly.