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

Call oncreate functions before updating bindings #694

Merged
merged 3 commits into from
Jul 7, 2017

Conversation

Rich-Harris
Copy link
Member

@TehShrike, as is his tendency, found a particularly baffling bug: when you have a binding to a child component's computed value, and the computed value is dependent on values that are set during oncreate, the observer infinite loop guard (callback.__calling) prevents any observers of the computed value from being called.

Quite possibly the mother of all edge cases, but a bug nonetheless. The solution seems to be to ensure that oncreate functions are called before parents are notified of the values of bindings to their children (since updating those bindings is what indirectly triggers the observers before they're 'ready').

Not 100% certain I've fully understood and articulated this bug, but it seems at least probable that this is the correct fix.

Also mentioned by @TehShrike but not addressed in this PR — the computed value in question is recomputed for each component that gets created. In the attached test, that means it's computed three times (once initially, then twice more for 'first thing' and 'second thing'). It would be nice to at least coalesce any computations that happen during oncreate, so that it only fires twice no matter how many components there are.

@Rich-Harris
Copy link
Member Author

Added a test that guarantees that a component's own bindings are updated before its oncreate function is called. I guess there might be a situation where things could happen in the wrong order? (In which case perhaps we would revert the swap, but prevent oncreate functions being called while bindings are still updating) I haven't managed to concoct a scenario in which that happens though, so I'm inclined to roll with this for now.

@PaulBGD
Copy link
Member

PaulBGD commented Jul 6, 2017

This makes me wonder if we should include more real world test cases, instead of a bunch of specific test cases. Like a large application, so we know if a change is breaking.

@TehShrike
Copy link
Member

hah! As far as bugs go, I'll just keep creating large real world apps, and the unit tests will grow naturally.

Now, if you want to start trying to write automated performance tests at some point (I have no experience with those), we could start looking around for bigger projects to pull in.

@Rich-Harris
Copy link
Member Author

Yeah, I tend to think that testing real-world applications makes more sense in theory than in practice — if you can anticipate all the possible failure modes, then you're better off just writing individual tests that cover them, and if you can't, then you're probably not going to be able to adequately test the application anyway. And keeping everything up to date could be rather awkward.

@Rich-Harris Rich-Harris merged commit 459ca95 into master Jul 7, 2017
@Rich-Harris Rich-Harris deleted the oncreate-before-bindings branch July 7, 2017 15:41
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.

3 participants