Skip to content
This repository has been archived by the owner on Dec 31, 2022. It is now read-only.

Feature: useDeferred(...) #18

Closed
wants to merge 2 commits into from

Conversation

YurkaninRyan
Copy link

@YurkaninRyan YurkaninRyan commented Jan 18, 2019

Hello there,

Thanks for this awesome project. I'm not used to what's going on here, but I tried my best to emulate the style and rules that i've seen throughout the project so far.

this PR adds tests and implementation for useDeferred. It returns a state-slotted object that contains a promise, along with its resolve and reject functions.

Closes #17

@@ -3,3 +3,4 @@
node_modules/
dist/
coverage/
.vscode/
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I normally have formatOnSave set to true, so for this repo specifically I set it to false. Didn't want to pollute it with my editor settings so I figured this would be the best course of action.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd instead add stuff to your .git/info/exclude or a user-global alternative.


function useDeferred() {
if (getCurrentBucket()) {
const [deferred] = useState((function () {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Area: Would love some insight on other ways to generate a Deferred object as an initial value. an IIFE that creates one was the best that I could come up with.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you can do this without support form upstream, what's the value of this being included here? You're setting yourself up for a leaking batteries problem, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the value in having this function implemented in this package. Can you help me understand why you see value in that?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derp, never mind me. Just realized that this was invited by getify on #17. I don't think it's wise to lump stuff together before there's good evidence for it being the default, but I also don't think it's wise to go against the maintainer's view.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: useDeferred()
2 participants