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

Improve docs for ky.stop #314

Merged
merged 9 commits into from
Jan 8, 2021

Conversation

ialexi-bl
Copy link
Contributor

Fixes #290. I would also like to add console.warn in shortcut methods, such as .json() and .text() that would display a warning when ky.stop is returned and these methods are used. Maybe a lazy solution to check if this result is not undefined could work:

ky/index.js

Line 338 in d976029

const result = isRetriableMethod ? this._retry(fn) : fn();

I can definitely add a check that it's not undefined to prevent those from throwing TypeError and to make them silently return undefined instead. The question is: is it guaranteed that if result is undefined, this was caused by ky.stop? If yes, I could also add console.warn in the same place where this check for result !== undefined is performed.

readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Described ky.stop in readme Improve docs for ky.stop Jan 4, 2021
@sindresorhus
Copy link
Owner

index.d.ts should be in sync with the readme.

@ialexi-bl
Copy link
Contributor Author

I would also suggest to change tabs in code snippets in readme to spaces, because they are pretty hard to read with many levels of nesting, considering how wide tabs on github are. Maybe in a different PR though

@sindresorhus
Copy link
Owner

I would also suggest to change tabs in code snippets in readme to spaces, because they are pretty hard to read with many levels of nesting, considering how wide tabs on github are. Maybe in a different PR though

No thanks :)

@sindresorhus sindresorhus merged commit 0ced1f1 into sindresorhus:master Jan 8, 2021
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.

Broken ky.stop
3 participants