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

[WIP] Refactor the options interface #38

Closed
wants to merge 1 commit into from
Closed

Conversation

SamVerschueren
Copy link
Contributor

I came across this solution in this thread. It's more experimental and maybe we can take some ideas from it.

However, it's not 100% identical to what we had. Because when using .reject(), the value is optional, which doesn't look like I can do easily with this approach.

A second approach could be to just do this

export interface Options {
	signal?: AbortSignal,
}

export interface ValueOptions<Value> {
	value: Value
}

declare const delay: {
	(milliseconds: number, options?: Options): ClearablePromise<void>;

	<Value>(milliseconds: number, options?: Options & ValueOptions<Value>): ClearablePromise<Value>;

	reject(milliseconds: number, options?: Options & Partial<ValueOptions<Value>>): ClearablePromise<never>;
};

Wrapping it in Partial makes all the properties of that interface optional.

I would be fine with keeping it as is though. Just wanted to share my ideas.

@felixfbecker
Copy link
Contributor

Personal opinion: I found it more readable before.

@sindresorhus
Copy link
Owner

@BendingBender Any opinion on this approach?

@BendingBender
Copy link
Contributor

BendingBender commented Apr 1, 2019

I'd rather do it like this. I don't find it inherently better, I find it just a bit clearer:

interface Options {
	signal?: AbortSignal;
	value?: unknown;
}

interface OptionsWithValue<ValueType> extends Options {
	value: ValueType;
}

type Delay = {
	// NOTE: In this overload the options parameter is not optional
	<ValueType>(milliseconds: number, options: OptionsWithValue<ValueType>): ClearablePromise<ValueType>;

	(milliseconds: number, options?: Options): ClearablePromise<void>;

	reject(milliseconds: number, options?: Options): ClearablePromise<never>;
};

@felixfbecker
Copy link
Contributor

Which is the same as right now, except with external interface and no specific docblock on value. Still prefer the current state 🤷‍♂️

@sindresorhus
Copy link
Owner

Yeah, I agree, what we have is better.

@sindresorhus sindresorhus deleted the omit-tsd branch April 8, 2019 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants