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

update _currentElement to nextElement to trigger correct event handlers #45

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

Conversation

davidmarkclements
Copy link
Contributor

it seems in some cases old event handlers from a prior render are triggered instead of the latest event handlers - this includes an event handler that accesses a scope reference, the value of the reference from a prior render applies instead of the most up to date reference (e.g. a boolean may have changed from true to false, but the old event handler still references the old true value).

This is due to this._currentElement being accessed in the event listener, but not being updated in cases where a re-render leads to the creation of a new element.

@davidmarkclements
Copy link
Contributor Author

btw this will cause a merge conflict with #38 - in this case we just need to take both sides of the conflict

@davidmarkclements
Copy link
Contributor Author

@Yomguithereal - would a hangouts session with explanations help to clear these PR's?

@Yomguithereal
Copy link
Owner

Just need time. Can you just tell me whether your open PRs are all still relevant and not conflicting with each other?

@davidmarkclements
Copy link
Contributor Author

no conflicts, other than the merge conflict noted above - all very much relevant I have a patches branch with them all working together https://github.com/davidmarkclements/react-blessed/tree/patches

@Yomguithereal
Copy link
Owner

Isn't setting this._currentElement here not dangerous? I don't recall react-dom doing this. This could have side-effects we did not foresee, no?

@davidmarkclements
Copy link
Contributor Author

potentially - I've not experienced any as yet.

If you don't do it there are already known side effects (firing old handlers) -

ideally we want to update the element in place instead of creating new elements, in which case this wouldn't be needed - could just use as a stop gap

@davidmarkclements
Copy link
Contributor Author

ping :)

@Yomguithereal
Copy link
Owner

So here we are speaking of an event triggered before rendering has been completed?

ideally we want to update the element in place instead of creating new elements, in which case this wouldn't be needed - could just use as a stop gap

Are the elements not updated but replaced?

@Yomguithereal
Copy link
Owner

I merged #38. Can you re-test the issue of this PR and tell me if this fix is still relevant please?

@Yomguithereal
Copy link
Owner

Any news @davidmarkclements?

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