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

withJob has workingProps state to ignore an old work's late resolve. #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dehypnosis
Copy link

To solve issue #45 (Need to abort an old work when a new work started).

I made a simple solution.

When new work starts, store the reference of current props object as wokringProps in the component.
And when job finished, compare the current props reference (in closure) with the stored wokringProps reference.
If references are not equal, just ignore the result as like ignore unmounted component
I'd like you to take a look at it.

@codecov
Copy link

codecov bot commented Nov 27, 2017

Codecov Report

Merging #47 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   94.17%   94.17%           
=======================================
  Files           5        5           
  Lines         103      103           
  Branches       31       31           
=======================================
  Hits           97       97           
  Misses          6        6
Impacted Files Coverage Δ
src/withJob.js 91.52% <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 6f99572...12110e2. Read the comment docs.

@dehypnosis dehypnosis changed the title withJob has workingProps state to ignore a old work's late resolve. withJob has workingProps state to ignore an old work's late resolve. Nov 27, 2017
@ctrlplusb
Copy link
Owner

Hey @dehypnosis!

Thanks for this. It's a really interesting case to cover.

I had a quick look and I think in principal it makes sense. One question though - do you think it would be safer to do a shallow equality check on the props object instead? Can we guarantee React always returns the same "wrapping" object holding the props?

What do you think?

I really appreciate the help 👍

@dehypnosis
Copy link
Author

dehypnosis commented Nov 27, 2017

resolveWork = (props) => {
  this.setState({ completed: false, data: null, error: null, workingProps: props })

...

if (isPromise(workDefinition)) {
    // Asynchronous result.
       return workDefinition
           .then((data) => {
               if (this.unmounted || this.state.workingProps !== props) {
...

Thank you for the answer.
In the submitted code, the comparison this.state.workingProps !== props is performed in the closure, so unless resolveWork called twice, props and workingProps will refer the same object naturally.

Did I rightly recognize your point?
As far as i know componentWillReceiveProps invokes only when props !== nextProps

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