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) - short-circuit diffing #1538

Closed
wants to merge 19 commits into from
Closed

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Apr 13, 2019

Libraries like redux and other state management libraries rely on this to propagate state changes.

Initial problem being that we pass the current vnode as old and new vnode on setState, this is logical but limits us in certain aspects for this case.

tests: #1322
Fixes: #1398 (This works now)

Adds: 15 14Bytes to the main package

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Apr 15, 2019

Can't seem to find the right approach for #1322 since the wrapper should bail out of rendering but it might send out something to Providers or?

The hard part is that when we are setting state we call diff with old and new set on the same entity which makes it kind off hard to do checks on this.

I've put this up to review with the test for 1322 skipped and 1398 being a success. I will handle 1322 in a seperate PR when I've gained more insight in this (or if anyone could give me a pointer would also be welcome).

@JoviDeCroock JoviDeCroock marked this pull request as ready for review April 15, 2019 19:54
@coveralls
Copy link

coveralls commented Apr 17, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 1cd9d74 on feat/shortCircuitDiff into f674187 on master.

@JoviDeCroock
Copy link
Member Author

JoviDeCroock commented Apr 17, 2019

Will try and debug this tomorrow... It seems that something broke because of the merge, _original and current seem to lose equality again

My guess is this: https://github.com/developit/preact/pull/1440/files#diff-a04f8653407a342eac2cd2ea6a5c3ff6R166 this could cause a double coercion

src/create-element.js Outdated Show resolved Hide resolved
@JoviDeCroock
Copy link
Member Author

I'm really struggling to find a reasoning when to short-circuit on setState and when not to. We could do a check if arguments are supplied or its just an empty object but it feels odd to even instantiate diffing for that. Is it just to activate a trickle-effect or? Someone that can shed more light on the use case of: #1322 ?

@JoviDeCroock
Copy link
Member Author

Hmm again errors after updating the branch, I'll leave this feature as it is until we get back to it.

@JoviDeCroock
Copy link
Member Author

So apparently something changed and when it bubbles it's considered equal now, this gets fixed by changing this line (possibleVNode._dom!=null || possibleVNode._component!=null) to not take _component in account.

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.

10.0.0-alpha.1 - Component returned from useMemo renders when nothing has changed
4 participants