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 reinitialize #62

Closed
wants to merge 3 commits into from
Closed

Feat reinitialize #62

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jul 16, 2019

Description

This PR resolves #58 which is about adding the reinitialize method.

Breaking changes

N\A

Checklist

Make sure you check all the boxes. You can omit items that are not applicable.

  • Implementation for both <Async> and useAsync()
  • Added / updated the unit tests
  • Added / updated the documentation
  • Updated the PropTypes
  • Updated the TypeScript type definitions

@codecov
Copy link

codecov bot commented Jul 16, 2019

Codecov Report

Merging #62 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #62      +/-   ##
==========================================
+ Coverage   98.69%   98.71%   +0.02%     
==========================================
  Files           9        9              
  Lines         691      703      +12     
  Branches      162      169       +7     
==========================================
+ Hits          682      694      +12     
  Misses          9        9
Impacted Files Coverage Δ
packages/react-async/src/propTypes.js 100% <ø> (ø) ⬆️
packages/react-async/src/Async.js 99.28% <100%> (+0.02%) ⬆️
packages/react-async/src/useAsync.js 97.89% <100%> (+0.06%) ⬆️
packages/react-async/src/reducer.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44ad0f8...a6cea6a. Read the comment docs.

Copy link
Member

@ghengeveld ghengeveld left a comment

Choose a reason for hiding this comment

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

Nice work, and thanks for the effort! 👍
I have just a couple of notes. Thanks for including tests, that's very helpful.

README.md Outdated Show resolved Hide resolved
packages/react-async/src/Async.js Outdated Show resolved Hide resolved
packages/react-async/src/reducer.js Outdated Show resolved Hide resolved
packages/react-async/src/useAsync.js Outdated Show resolved Hide resolved
@ghost ghost closed this Jul 17, 2019
@ghost ghost deleted the feat-reinitialize branch July 17, 2019 14:47
@ghengeveld
Copy link
Member

@all-contributors please add @noelyoo for code, ideas

@allcontributors
Copy link
Contributor

@ghengeveld

I've put up a pull request to add @noelyoo! 🎉

@maximenathan
Copy link

Hello

I would like to know why this PR was closed?
The Issue 58 doesn't explain it very well.
Thank you!

@ghengeveld
Copy link
Member

Hi @maximenathan. The reason this was closed is because the implementation that @neutiyoo came up with did not match the mental model of reinitializing the component as if it was destroyed and recreated. After some discussion in this thread he decided it would be best not to merge this PR because it would only complicate the library while there's an alternative solution available (for his use case at least).

If you are in need of this feature, can you explain your use case for it? If there's a good use case without simple enough workaround then I'm happy to add the feature.

@maximenathan
Copy link

Hi @ghengeveld thanks for your quick response and sorry for the delay
I found a workaround. I will add more information if it gets more tricky
Thank you for your work! :)

This pull request was closed.
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.

2 participants