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

feat!: introduce controls option #362

Merged
merged 4 commits into from
May 11, 2021
Merged

feat!: introduce controls option #362

merged 4 commits into from
May 11, 2021

Conversation

antfu
Copy link
Member

@antfu antfu commented Mar 5, 2021

Motivation

Returning object allows functions have more flexibility and extensibility in the future. But for most of the time, we might just need one value without that many controls. The renaming in object destructure can be a bit verbose.

const { timestamp: myTimestamp } = useTimestamp()

Proposed Solution

Introduce a new controls option that changes the return type. By default, controls should be false make the usage more concise while also make it possible to have more controls when needed.

const myTimestamp = useTimestamp()
const { timestamp, pause, resume } = useTimestamp({ controls: true })

It's well-typed in TypeScript, this should be able to minimize the confusion for both JS/TS users.

Affect functions

This is breaking changes for some functions. But we have different returning types (object/single ref) for different functions, this proposal also unified them to make the API more consistent.

  • useNow
  • useTimestamp
  • useTimeAgo
  • useInterval
  • useTimeout

API Changes

  • useIntervalFn - removed start stop function (previously deprecated)
  • useTimeoutFn - renamed isActive to isPending
  • useTimeoutFn, useIntervalFn - last argument changed to options object

@antfu
Copy link
Member Author

antfu commented Mar 6, 2021

@vueuse/collaborators Would love to hear what you think about this, thanks!

This is a breaking change aiming to be released to in v5.0. If you got any idea of refactoring/redesign other existing function, please let me know :)

@wheatjs
Copy link
Member

wheatjs commented Mar 6, 2021

I think this will simplify usage for a majority of use cases for these functions. As for other functions that might make use of this I'll need to go through the list to see if I find any good candidates. Anyway really good work, I like this change 😄👍

@patak-dev
Copy link
Member

I just checked useRefHistory, useManualRefHistory, ignorableWatch and pausableWath to see if they could benefit from the new { controls: true } pattern. I think that in all of them, the most common case is to use the controls (I would argue that you would always need them. There may be a case where you only would like to use it for the history array... but I think that if users would want that, they are better using a simpler function). So IMO, we are good leaving these four with the current API 👍🏼

@patak-dev
Copy link
Member

I like this idea, this comment is only about how to select the controlled vs uncontrolled version.

@antfu did you consider using a different function instead of passing the controls boolean?

Internally it could still be implemented using this scheme in case it makes sense to reuse the implementation, but having a different function could give you the flexibility to simplify the uncontrolled versions of some of these functions (shaving some bytes and avoiding some extra objects in memory). For example, you could avoid the need for the isActive ref since there is no longer way to pause the timers:

const myTimestamp = useTimestamp()
const { timestamp, pause, resume } = usePausableTimestamp()

(Note: Using Pausable because there are already some functions using this naming scheme, didn't though too much about it.)

@scottbedard
Copy link
Member

I love this idea! I will definitely apply this in my revisit of useTransition, there are things like the pause / resume functions that would be great to expose!

@antfu
Copy link
Member Author

antfu commented Mar 7, 2021

@matias-capeletto Thanks for the feedback! Actually, expose a new function for different signatures might be the more "right" way to do. But here is a few reasons I think it's better to go with controls.

  • Exposing more functions creates a larger maintaining surface
  • Most of those functions rely on the underneath controls, so in the most of the case, we will still need to bundle those code, no matter you use the controlled or uncontrolled versions.
  • With controls, users could go with the uncontrolled version first, and when they think "Oh I need more controls over it", simply pass the boolean and it will do the job. On the other hand, with the functions approach, it will require users to find the corresponding function name. Unless we make uniform conventions, it could be also hard and confusing for users to understand the mapping, useTimestamp -> usePausableTimestamp and useTransition -> useControlledTransition
  • For choosing the defaults, I think we can judge them by a rule - "if the controls are required to use the functionality". For useRefHistory-ish, I think it's reasonable to keep them as-is as you said. It's kinda weird to have history without the undo-redo capability.

WDYT?

@patak-dev
Copy link
Member

Good points, totally fine with using { controls: true }.

About the first two points, you are right. I would say that is more about internal implementation, the docs could still remain the same because you could bundle the two functions in a single page (I think it is roughly the same to explain both ways). I think it is about trade-offs, as usual. Probably users that care a lot about performance are going to implement the timestamp themselves, and you can say that VueUse favors usability.

About naming, I first went with useControlledTransition, but controlledRef means something totally different so I think it will be confusing. You are right that uniform naming is important, maybe useTransitionWithControls could be a good name.

The other problem I see is there is no way to tell Typescript that the two functions are related and that it should show them to the user together (Imagine if when you are using useTransition, you will get useTransitionWithControls listed when you are writing the params somehow helping users to discover it). So this is a +1 for { controls: true }.

@lstoeferle
Copy link
Member

I like the idea! 👏 Like @matias-capeletto, I see VueUse as an usability first library that helps people to build stuff more easily without deep knowledge about some browser APIs for example. Therefore I don't have any objections against it.

@wheatjs
Copy link
Member

wheatjs commented Mar 11, 2021

Since this version is going to introduce breaking changes, perhaps we could update the useRafFn while we are at it. I'm not sure how much it will benefit from the controls option, since besides pause and resume it only returns isActive, but we could remove stop and start since they have already been deprecated.

https://github.com/vueuse/vueuse/blob/main/packages/core/useRafFn/index.ts

@antfu
Copy link
Member Author

antfu commented Mar 11, 2021

@jacobclevenger Thanks for spotting this. For sure we should remove the deprecated APIs. I don't think we would need controls for it tho, as people would normally use it without return values. I will update in a sec.

@wheatjs
Copy link
Member

wheatjs commented Mar 12, 2021

Also looks like useAsyncState has a deprecated API that could be removed

/** @deprecated, use isReady instead */
ready: isReady,

@antfu antfu marked this pull request as ready for review May 11, 2021 18:23
@antfu antfu merged commit 44eba11 into next May 11, 2021
@antfu antfu deleted the feat/controls branch May 11, 2021 18:23
@antfu antfu mentioned this pull request May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants